Refactor for SignalManager changes #277
Conversation
|
@ngondalia I'd like to know if the new typing inside |
| this.handleUpdatedProperties(emptyPayload); | ||
| } | ||
| handleExperimentChanged = (exp?: Experiment) => { | ||
| const payload = exp ? this.propertiesMap.get(exp.UUID) : new ItemPropertiesSignalPayload({}); |
There was a problem hiding this comment.
I am getting 'TypeError: Cannot read properties of undefined (reading 'getExperimentUUID')'
The payload could be undefined here - Either have to address it here or could handle it in handleUpdatedProperties
There was a problem hiding this comment.
Yea, there was a weird issue where when exp is Experiment | undfined you can't pass string | undefined into the getter. So i changed the logic but made the assumption we would always find something if exp was defined..
Anyway... I added another checker so if this.propertiesMap.get(exp.UUID) doesn't find anything we just put an empty ItemPropertiesSignalPayload which should fix that.
williamsyang-work
force-pushed
the
signal-type-enforcement
branch
from
fa29fc0 to
e68dbf4
Compare
|
@williamsyang-work could you please rebase this PR and fix the linter issues. There is a merge conflict with a recent change. Meanwhile I re-review the corresponding theia-trace-extension PR. |
Sure. I had this as a draft until the theia-extension PR was merged. I will tidy this one up for proper review. |
williamsyang-work
force-pushed
the
signal-type-enforcement
branch
from
e68dbf4 to
301343b
Compare
Replaces Signal keys with strings for event handlers Replaces fire* methods with emit methods Signed-off-by: Will Yang <[email protected]>
williamsyang-work
force-pushed
the
signal-type-enforcement
branch
from
301343b to
154f7cb
Compare
There was a problem hiding this comment.
The trace-viewer components have been released with version v0.5.0. (eclipse-cdt-cloud/theia-trace-extension#1159). Please update this PR to upgrade to that version. Rrefer also #288 to see which files to update for the upgrade.
Signed-off-by: William Yang <[email protected]>
|
The following IP tickets have been created for the upgrade to traceviewer components v0.5.0: |
There was a problem hiding this comment.
I just realized that the README.md is describing the external API and it needs to be updated. Please go through the following chapter and update it the new API. The API is shown there and there is an example for the experiment selected signal as far I can see.
Signed-off-by: William Yang <[email protected]>
williamsyang-work
force-pushed
the
signal-type-enforcement
branch
from
cf37aaf to
dc905e2
Compare
marcdumais-work
requested review from
marcdumais-work
and removed request for
ngondalia
|
There's an extra dependency requiring an IP Ticket that popped-up (it was not mentioned in the previous CI run):
IP ticket: |
There was a problem hiding this comment.
LGTM! Really nice contribution @williamsyang-work! (specially when considering the related ground work you first did in the theia-trace-extension repo)
I do not think we need for this ticket to be resolved before merging - I had a look and I am 99.9% sure this License check failure is a false positive. |
|
I'm going to merge this change. Then, I plan to do a release this week. @ngondalia please let me know if you have any concern about the release. |
IP Ticket closed (approved) |
Thanks for the heads up! I will do sanity testing on our end and will get back to you. |
Replaces Signal keys with strings for event handlers Replaces fire* methods with emit methods