chore: upgrade eventemitter to v5.0.2 #7709

ozyx · 2024-05-02T00:01:02Z

Closes #4656
Closes #6426

Describe your changes:

Upgrades eventemitter3 library to the latest version 5.0.2.

Removes unused event listeners (registering listeners without context or unregistering non-existent listeners now raises an exception).
Converts DrawWebGL and Draw2D to ES6 classes so they can inherit from EventEmitter without losing context.
Includes the context in listeners registered with eventHelpers

All Submissions:

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?
Is this a notable change that will require a special callout in the release notes? For example, will this break compatibility with existing APIs or projects that consume these plugins?

Author Checklist

Changes address original issue?
Tests included and/or updated with changes?
Has this been smoke tested?
Have you associated this PR with a type: label? Note: this is not necessarily the same as the original issue.
Have you associated a milestone with this PR? Note: leave blank if unsure.
Testing instructions included in associated issue OR is this a dependency/testcase change?

Reviewer Checklist

Changes appear to address issue?
Reviewer has tested changes by following the provided instructions?
Changes appear not to be breaking changes?
Appropriate automated tests included?
Code style and in-line documentation are appropriate?

ozyx · 2024-05-02T00:01:56Z

src/plugins/LADTable/components/LadRow.vue

@@ -237,13 +237,11 @@ export default {
    }

    this.previewAction = new PreviewAction(this.openmct);
-    this.previewAction.on('isVisible', this.togglePreviewState);


this.togglePreviewState was undefined, and this wasn't doing anything except making our unit tests fail. It seems that the new version throws an exception if you try to unregister a listener that doesn't exist, or register a listener without context.

I think you're right. I wonder if our preview action changed at some point as it doesn't seem to have much benefit to emitting this 'isVisible' event at all.

It seems that the new version throws an exception if you try to unregister a listener that doesn't exist, or register a listener without context.

From v3.0.0 onwards, EventEmitter3 throws an error if you try to register a listener with a callback that is not a function

Actually, this is being used for search results here:
https://github.com/nasa/openmct/blob/master/src/ui/layout/search/SearchResultsDropDown.vue#L48
It was likely a copy-paste error where the this.togglePreviewState method was not added. It is also missing in AnnotationSearchResult.vue

You can see an implementation here:
https://github.com/nasa/openmct/blob/master/src/ui/layout/RecentObjectsListItem.vue#L116

src/ui/layout/RecentObjectsListItem.vue

codecov-commenter · 2024-05-02T00:05:39Z

Codecov Report

Attention: Patch coverage is 74.24893% with 60 lines in your changes are missing coverage. Please review.

Project coverage is 54.87%. Comparing base (b3d3465) to head (e12ea9c).

❗ Current head e12ea9c differs from pull request most recent head c323b5b. Consider uploading reports for the commit c323b5b to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7709      +/-   ##
==========================================
- Coverage   56.81%   54.87%   -1.95%     
==========================================
  Files         673      673              
  Lines       27185    27181       -4     
  Branches     2635     2635              
==========================================
- Hits        15445    14915     -530     
- Misses      11412    11936     +524     
- Partials      328      330       +2

Flag	Coverage Δ
e2e-full	`23.56% <26.21%> (-18.23%)`	⬇️
e2e-stable	`53.76% <69.41%> (-6.23%)`	⬇️
unit	`49.20% <60.94%> (-0.18%)`	⬇️

Files	Coverage Δ
src/MCT.js	`96.93% <ø> (-0.07%)`	⬇️
src/plugins/LADTable/components/LadRow.vue	`55.78% <100.00%> (+0.11%)`	⬆️
src/plugins/clearData/ClearDataAction.js	`96.42% <100.00%> (+0.13%)`	⬆️
...gins/condition/components/inspector/StylesView.vue	`49.85% <100.00%> (ø)`
...ins/displayLayout/actions/CopyToClipboardAction.js	`72.22% <100.00%> (+1.63%)`	⬆️
src/plugins/duplicate/DuplicateAction.js	`29.31% <100.00%> (-0.52%)`	⬇️
...c/plugins/exportAsJSONAction/ExportAsJSONAction.js	`82.16% <100.00%> (-0.53%)`	⬇️
src/plugins/formActions/CreateAction.js	`93.75% <100.00%> (-3.08%)`	⬇️
src/plugins/formActions/EditPropertiesAction.js	`93.93% <100.00%> (+0.18%)`	⬆️
src/plugins/formActions/plugin.js	`100.00% <100.00%> (ø)`
... and 38 more

... and 81 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3d3465...c323b5b. Read the comment docs.

src/plugins/imagery/lib/eventHelpers.js

davetsay

I just had a couple minor suggestions but really good.

src/plugins/plot/chart/MctChart.vue

src/ui/layout/RecentObjectsListItem.vue

…down

akhenry · 2024-05-06T23:10:14Z

src/MCT.js

@@ -79,7 +79,6 @@ import Browse from './ui/router/Browse.js';
 export class MCT extends EventEmitter {
  constructor() {
    super();
-    EventEmitter.call(this);


Whaaaat were we even doing here? Suspect this was a hangover from prototypical inheritance that was left in by mistake.

Yup, exactly.

akhenry · 2024-05-06T23:11:36Z

src/plugins/imagery/lib/eventHelpers.js

@@ -41,7 +41,7 @@ const helperFunctions = {
    } else if (object.addEventListener) {
      object.addEventListener(event, listener._cb);
    } else {
-      object.on(event, listener._cb);


Wait, so how was this working before? The context would have been undefined?

akhenry

Changes look good to me. My only question is all the cases where we are now providing a context object for the callback function. Why is that suddenly necessary, and how was it working previously with an undefined context?

unlikelyzero

Need to confirm with Jamie and David

unlikelyzero · 2024-05-07T02:16:27Z

src/plugins/tabs/components/TabsComponent.vue

@@ -171,7 +170,6 @@ export default {
    this.updateCurrentTab = this.updateCurrentTab.bind(this);
    this.openmct.router.on('change:params', this.updateCurrentTab);

-    this.RemoveAction = new RemoveAction(this.openmct);


It was never used

unlikelyzero · 2024-05-07T02:18:44Z

src/ui/components/ObjectLabel.vue

@@ -50,11 +50,12 @@
 <script>
 import { inject } from 'vue';

+import { PREVIEW_ACTION_KEY } from '@/ui/preview/PreviewAction.js';


I don't think we can have aliased imports due to the JPL codebase @davetsay @jvigliotta

This is only true for files that they import directly from source (🙈). @jvigliotta @davetsay which files are we honor-bound to keep webpack-alias-free?

Good looking out, @unlikelyzero . We'll fix this on the OMM side. Core should not worry about using or not using aliased imports. As long as we use the same webpack aliases in OMM it will work (🙈+1).

Co-authored-by: David Tsay <3614296+davetsay@users.noreply.github.com>

ozyx · 2024-05-07T16:12:03Z

Changes look good to me. My only question is all the cases where we are now providing a context object for the callback function. Why is that suddenly necessary, and how was it working previously with an undefined context?

In v1.2.0 (and beyond), if no context is provided, it will fall back to using the EventEmitter context, which seemed to work for our purposes before. As a part of these changes, I converted Draw2D and DrawWebGL to ES6 classes, so the default context there becomes the class instance instead of the caller. So we need to explicitly pass the context to the EE to preserve the previous behavior.

davetsay

Great work, @ozyx. Definitely more worthy work than chore gives it credit for.

akhenry · 2024-05-08T15:39:41Z

src/ui/preview/PreviewHeader.vue

@@ -107,23 +111,33 @@ export default {
    };
  },
  watch: {
-    async currentView() {
-      // wait for view to render with next tick
-      await nextTick();


Gross, thank you.

akhenry · 2024-05-08T15:54:39Z

Deferring to post RC5

ozyx · 2024-05-14T17:43:31Z

Deferring to post RC5

let's merge?

chore: upgrade eventemitter to v5.0.2

dc77e35

ozyx added this to the Target:4.0.0 milestone May 2, 2024

ozyx added the type:maintenance tests, chores, or project maintenance label May 2, 2024

ozyx commented May 2, 2024

View reviewed changes

src/ui/layout/RecentObjectsListItem.vue Outdated Show resolved Hide resolved

ozyx added 3 commits May 1, 2024 21:48

fix: pass context to eventHelpers

7df3d66

lint: remove unused imports

8fc2cce

lint: prettier

bede498

ozyx added the pr:e2e:couchdb npm run test:e2e:couchdb label May 2, 2024

github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label May 2, 2024

ozyx added the performance impacts or improves performance label May 2, 2024

ozyx marked this pull request as ready for review May 2, 2024 16:09

ozyx requested review from davetsay, akhenry and unlikelyzero May 2, 2024 16:10

unlikelyzero approved these changes May 3, 2024

View reviewed changes

ozyx added the pr:e2e:perf Trigger perf tests label May 3, 2024

davetsay reviewed May 3, 2024

View reviewed changes

src/plugins/imagery/lib/eventHelpers.js Show resolved Hide resolved

github-actions bot removed the pr:e2e:perf Trigger perf tests label May 3, 2024

davetsay requested changes May 3, 2024

View reviewed changes

src/plugins/plot/chart/MctChart.vue Outdated Show resolved Hide resolved

src/ui/layout/RecentObjectsListItem.vue Outdated Show resolved Hide resolved

ozyx added 5 commits May 6, 2024 15:06

fix: no need to destroy router as it is destroyed during openmct tear…

caa398f

…down

fix(test): ensure sourcemap settings align

a394dcc

refactor: export action key consts

2396cc7

fix: register CreateAction and retrieve it from the registry

019be8d

test: fix tests

72d3ecb

akhenry reviewed May 6, 2024

View reviewed changes

akhenry requested changes May 6, 2024

View reviewed changes

refactor: import action key consts

58c9101

ozyx added 3 commits May 6, 2024 16:22

refactor: remove unused import

5dd2565

fix: update usage. don't use getters

71a3cc2

refactor: const keys for the other actions

a3c3174

unlikelyzero reviewed May 7, 2024

View reviewed changes

refactor: use optional chaining

c323b5b

Co-authored-by: David Tsay <3614296+davetsay@users.noreply.github.com>

ozyx force-pushed the upgrade-eventemitter3 branch from e12ea9c to c323b5b Compare May 7, 2024 05:12

ozyx requested review from akhenry and davetsay May 7, 2024 17:34

davetsay approved these changes May 7, 2024

View reviewed changes

akhenry reviewed May 8, 2024

View reviewed changes

Merge branch 'master' into upgrade-eventemitter3

2fa20ef

akhenry approved these changes May 14, 2024

View reviewed changes

akhenry enabled auto-merge (squash) May 14, 2024 17:47

ozyx added the pr:e2e:couchdb npm run test:e2e:couchdb label May 14, 2024

github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label May 14, 2024

akhenry merged commit 017380b into master May 14, 2024
27 of 34 checks passed

akhenry deleted the upgrade-eventemitter3 branch May 14, 2024 21:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: upgrade eventemitter to v5.0.2 #7709

chore: upgrade eventemitter to v5.0.2 #7709

ozyx commented May 2, 2024 •

edited by akhenry

ozyx May 2, 2024 •

edited

davetsay May 3, 2024

ozyx May 7, 2024

shefalijoshi May 8, 2024

codecov-commenter commented May 2, 2024 •

edited

davetsay left a comment

akhenry May 6, 2024

ozyx May 7, 2024

akhenry May 6, 2024

akhenry left a comment

unlikelyzero left a comment

unlikelyzero May 7, 2024

ozyx May 7, 2024

unlikelyzero May 7, 2024

ozyx May 7, 2024

davetsay May 7, 2024

ozyx commented May 7, 2024

davetsay left a comment

akhenry May 8, 2024

akhenry commented May 8, 2024

ozyx commented May 14, 2024

chore: upgrade eventemitter to v5.0.2 #7709

chore: upgrade eventemitter to v5.0.2 #7709

Conversation

ozyx commented May 2, 2024 • edited by akhenry

Describe your changes:

All Submissions:

Author Checklist

Reviewer Checklist

ozyx May 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 2, 2024 • edited

Codecov Report

davetsay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akhenry left a comment

Choose a reason for hiding this comment

unlikelyzero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ozyx commented May 7, 2024

davetsay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akhenry commented May 8, 2024

ozyx commented May 14, 2024

ozyx commented May 2, 2024 •

edited by akhenry

ozyx May 2, 2024 •

edited

codecov-commenter commented May 2, 2024 •

edited