chore: remove `type: module`, create `openmct-e2e` subpackage #7590

ozyx · 2024-03-14T20:37:27Z

Describe your changes:

Now this is a story all about how our project got flipped, turned upside down--

TL;DR: We took openmct to type: 'module' too soon.

A few months back, we converted the entire project to using ESM import/export syntax. We switched the package itself to type: module because running unit/e2e tests were failing (as we couldn't use import or export keywords unless the package was of type: module or the file had an *.mjs extension).

Even though our package was of type: module, under the hood Webpack was compiling our source using a UMD wrapper which is meant to support imports in both node and browser environments. However, this never worked properly because we set the package type as 'module' (when it really wasn't!). Consumers would attempt to import the package only to find that it defined no exports. They would then try to require the package, only to be denied under the basis that you cannot require an ESModule. In fact, webpack currently does not support packaging a library as an ESModule. As it stands, webpack is a dead-end for openmct as a true ESModule.

This PR fixes a ton of misconfiguration with webpack, switches our package type off of module, and specifies paths and entry points for both import and require syntax.

In addition, and to avoid having to change the extensions of all e2e test files to *.mjs, this PR initializes a new npm sub-package openmct-e2e of type: module. This package is specified as a workspace of openmct, so a single node_modules folder is shared between the two projects (same as it ever was).

See the corresponding PRs for openmct-yamcs and viper-openmct.

Brief Summary of Changes

Removes e2e folder from our distributable
Removes type: module from package.json
Initializes a new package openmct-e2e which exports our e2e testing framework
Adds e2e folder as a workspace to openmct so they share node_modules
webpack.*.js -> webpack.*.mjs
Specifies module and exports in package.json
Updates npm e2e test scripts to use --workspace to call scripts in e2e folder
Modifies .tsconfig.json settings to generate TypeScript types in dist/types/index.d.ts as opposed to creating individual files.

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?

codecov · 2024-03-14T20:41:55Z

Codecov Report

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

Project coverage is 49.99%. Comparing base (fb396ac) to head (b45a127).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7590      +/-   ##
==========================================
- Coverage   56.13%   49.99%   -6.15%     
==========================================
  Files         672      672              
  Lines       27116    27111       -5     
  Branches     2635     2635              
==========================================
- Hits        15222    13554    -1668     
- Misses      11567    13230    +1663     
  Partials      327      327

Flag	Coverage Δ
e2e-full	`23.57% <ø> (-0.03%)`	⬇️
e2e-stable	`∅ <ø> (∅)`
unit	`49.11% <0.00%> (+0.02%)`	⬆️

Files	Coverage Δ
src/plugins/notebook/utils/painterroInstance.js	`3.70% <0.00%> (-85.19%)`	⬇️

... and 189 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 fb396ac...b45a127. Read the comment docs.

package.json

shefalijoshi · 2024-03-14T21:47:53Z

@ozyx This is perfect! I tried it with the nirvss project and it now works really well. Thank you!

package.json

tsconfig.json

scottbell

Nice work! We'll need to let others know who use Open MCT that this will require them updating how they import us. I tested core, and using the accompanying PR, and everything appeared to work well.

codecov-commenter · 2024-03-26T00:16:40Z

Codecov Report

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

Project coverage is 49.99%. Comparing base (a5c6b14) to head (1de1510).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7590      +/-   ##
==========================================
- Coverage   56.60%   49.99%   -6.62%     
==========================================
  Files         672      672              
  Lines       27120    27120              
  Branches     2632     2632              
==========================================
- Hits        15351    13558    -1793     
- Misses      11441    13234    +1793     
  Partials      328      328

Flag	Coverage Δ		*Carryforward flag
e2e-full	`23.64% <ø> (-18.13%)`	⬇️	Carriedforward from c8a7114
e2e-stable	`?`
unit	`49.08% <0.00%> (-0.03%)`	⬇️

*This pull request uses carry forward flags. Click here to find out more.

Files	Coverage Δ
src/plugins/notebook/utils/painterroInstance.js	`3.70% <0.00%> (-85.19%)`	⬇️

... and 192 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 a5c6b14...1de1510. Read the comment docs.

e2e/tests/functional/plugins/clocks/remoteClock.e2e.spec.mjs

e2e/tests/functional/plugins/importAndExportAsJSON/importAsJson.e2e.spec.mjs

akhenry

Changes look great. Looks like there are some new conflicts due to all the recent merging, and two github warnings we should address before merge. Thanks!

e2e/README.md

e2e/test-data/overlay_plot_storage.json

e2e/tests/functional/planning/ganttChart.e2e.spec.mjs

e2e/tests/functional/plugins/clocks/remoteClock.e2e.spec.mjs

e2e/tests/functional/plugins/importAndExportAsJSON/exportAsJson.e2e.spec.mjs

e2e/tests/functional/plugins/importAndExportAsJSON/importAsJson.e2e.spec.mjs

openmct.js

src/plugins/notebook/utils/painterroInstance.js

package.json

- we aren't a module... yet

…fix imports

… subpackage

This reverts commit eb14d52.

e2e/index.js

unlikelyzero · 2024-03-27T22:26:01Z

e2e/package.json

@@ -0,0 +1,27 @@
+{
+  "name": "openmct-e2e",
+  "version": "4.0.0-next",


this should always match openmct's package.json. Is there an easy way to manage that based on monorepo tooling?

unlikelyzero · 2024-03-27T22:26:47Z

e2e/playwright-ci.config.js

@@ -15,6 +15,7 @@ const config = {
  timeout: 60 * 1000,
  webServer: {


We need a .npmignore file and to ignore *.config.js

unlikelyzero

Two small changes:

We should not be exposing all avpFixtures in other projects. viper should never attempt to publish a percy check!
We need to .npmignore our e2e config files

unlikelyzero · 2024-03-27T23:59:58Z

@ozyx just needs to add npmignore for the config files. avpFixtures is not a problem

unlikelyzero · 2024-03-28T01:38:01Z

packages/e2e/.npmignore

@@ -0,0 +1,7 @@
+*
+!appActions.js


I think we're going to want to include the test s but not right now

bad origin on the initial review. All comments were already addressed

shefalijoshi reviewed Mar 14, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

ozyx force-pushed the fix-import-require branch from 3248694 to 14ff6b1 Compare March 15, 2024 21:18

ozyx requested a review from unlikelyzero March 15, 2024 21:18

ozyx added this to the Target:4.0.0 milestone Mar 15, 2024

ozyx added the type:maintenance tests, chores, or project maintenance label Mar 15, 2024

ozyx requested a review from akhenry March 16, 2024 18:09

ozyx changed the title ~~Fix import require~~ chore: remove type: module, create openmct-e2e subpackage Mar 16, 2024

ozyx marked this pull request as ready for review March 16, 2024 18:10

ozyx requested a review from scottbell March 16, 2024 18:12

ozyx added the pr:e2e:couchdb npm run test:e2e:couchdb label Mar 16, 2024

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

scottbell reviewed Mar 19, 2024

View reviewed changes

package.json Show resolved Hide resolved

scottbell reviewed Mar 19, 2024

View reviewed changes

tsconfig.json Show resolved Hide resolved

scottbell approved these changes Mar 19, 2024

View reviewed changes

ozyx removed this from the Target:4.0.0 milestone Mar 19, 2024

ozyx marked this pull request as draft March 19, 2024 20:44

ozyx added this to the Target:4.0.1 milestone Mar 21, 2024

ozyx force-pushed the fix-import-require branch from b45a127 to 5f9e83d Compare March 26, 2024 00:16

ozyx marked this pull request as ready for review March 26, 2024 00:16

github-advanced-security bot found potential problems Mar 26, 2024

View reviewed changes

e2e/tests/functional/plugins/clocks/remoteClock.e2e.spec.mjs Fixed Show resolved Hide resolved

e2e/tests/functional/plugins/importAndExportAsJSON/importAsJson.e2e.spec.mjs Fixed Show resolved Hide resolved

akhenry previously requested changes Mar 27, 2024

View reviewed changes

ozyx force-pushed the fix-import-require branch from 78f6e88 to 69ae299 Compare March 27, 2024 20:46

ozyx requested a review from akhenry March 27, 2024 20:46

ozyx added 4 commits March 27, 2024 13:49

fix: remove mystery webpack code

e92e3e7

fix: remove type:module and specify exports

943f61b

- we aren't a module... yet

fix: rename webpack*.js to webpack*.mjs so we can use import/export. …

70fe7b4

…fix imports

fix: exports format

c83d25f

ozyx added 9 commits March 27, 2024 13:56

chore: compile types to a single file

bf9ccf7

chore: fix visual test npm scripts

8f63a44

chore: fix import pathing

96de064

chore: define package exports, move test specific dependencies to the…

352ef77

… subpackage

chore: export test framework from openmct-e2e

162f8d6

chore: export baseFixtures also

763cf6e

chore: let openmct and openmct-e2e share node_modules/

e790a9a

chore: use --workspace, remove pretest script

ebe00fd

Revert "fix: remove mystery webpack code"

a42daad

This reverts commit eb14d52.

ozyx force-pushed the fix-import-require branch from 69ae299 to a42daad Compare March 27, 2024 21:13

chore: update package-lock

f65ea83

unlikelyzero reviewed Mar 27, 2024

View reviewed changes

e2e/index.js Show resolved Hide resolved

unlikelyzero reviewed Mar 27, 2024

View reviewed changes

unlikelyzero requested changes Mar 27, 2024

View reviewed changes

unlikelyzero modified the milestones: Target:4.0.1, Target:4.0.0 Mar 27, 2024

unlikelyzero approved these changes Mar 27, 2024

View reviewed changes

chore: add .npmignore

3d4c9d9

unlikelyzero approved these changes Mar 28, 2024

View reviewed changes

ozyx added 2 commits March 28, 2024 13:48

Merge branch 'master' into fix-import-require-the-other-way

c8a7114

fix: *js -> mjs

1de1510

ozyx added the pr:e2e:couchdb npm run test:e2e:couchdb label Mar 28, 2024

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

ozyx mentioned this pull request Mar 28, 2024

Require/import of openmct doesn't work as expected #7602

Closed

unlikelyzero merged commit a3fb84a into master Mar 28, 2024
23 of 25 checks passed

unlikelyzero deleted the fix-import-require branch March 28, 2024 21:49

ozyx added the notable_change A change which should be noted in the changelog label Apr 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: remove `type: module`, create `openmct-e2e` subpackage #7590

chore: remove `type: module`, create `openmct-e2e` subpackage #7590

ozyx commented Mar 14, 2024 •

edited

codecov bot commented Mar 14, 2024 •

edited

shefalijoshi commented Mar 14, 2024

scottbell left a comment

codecov-commenter commented Mar 26, 2024 •

edited

akhenry left a comment

unlikelyzero Mar 27, 2024

unlikelyzero Mar 27, 2024 •

edited

unlikelyzero left a comment

unlikelyzero commented Mar 27, 2024

unlikelyzero Mar 28, 2024

		@@ -15,6 +15,7 @@ const config = {
		timeout: 60 * 1000,
		webServer: {

chore: remove type: module, create openmct-e2e subpackage #7590

chore: remove type: module, create openmct-e2e subpackage #7590

Conversation

ozyx commented Mar 14, 2024 • edited

Describe your changes:

Brief Summary of Changes

All Submissions:

Author Checklist

Reviewer Checklist

codecov bot commented Mar 14, 2024 • edited

Codecov Report

shefalijoshi commented Mar 14, 2024

scottbell left a comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 26, 2024 • edited

Codecov Report

akhenry left a comment

Choose a reason for hiding this comment

unlikelyzero Mar 27, 2024

Choose a reason for hiding this comment

unlikelyzero Mar 27, 2024 • edited

Choose a reason for hiding this comment

unlikelyzero left a comment

Choose a reason for hiding this comment

unlikelyzero commented Mar 27, 2024

unlikelyzero Mar 28, 2024

Choose a reason for hiding this comment

chore: remove `type: module`, create `openmct-e2e` subpackage #7590

chore: remove `type: module`, create `openmct-e2e` subpackage #7590

ozyx commented Mar 14, 2024 •

edited

codecov bot commented Mar 14, 2024 •

edited

codecov-commenter commented Mar 26, 2024 •

edited

unlikelyzero Mar 27, 2024 •

edited