Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pure ES Modules break the Mocha tests #3779

Open
eatyourgreens opened this issue Oct 14, 2022 · 32 comments
Open

Pure ES Modules break the Mocha tests #3779

eatyourgreens opened this issue Oct 14, 2022 · 32 comments
Labels

Comments

@eatyourgreens
Copy link
Contributor

eatyourgreens commented Oct 14, 2022

Updated 2024 - List of third-party packages this applies to:

(Dependabot has been told to ignore the major version upgrades)


Originally posted by @shaunanoordin in #3361 (comment) The tests for app-project and lib-classifier are failing on:

Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/shaun/projects/front-end-monorepo/node_modules/d3/src/index.js from /Users/shaun/projects/front-end-monorepo/packages/app-project/src/shared/components/ProjectStatistics/components/CompletionBar/CompletionBar.js not supported.
Instead change the require of index.js in /Users/shaun/projects/front-end-monorepo/packages/app-project/src/shared/components/ProjectStatistics/components/CompletionBar/CompletionBar.js to a dynamic import() which is available in all CommonJS modules.

and

Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/shaun/projects/front-end-monorepo/node_modules/d3/src/index.js from /Users/shaun/projects/front-end-monorepo/packages/lib-classifier/src/components/Classifier/components/Feedback/components/Graph2dRangeFeedback.js not supported.
Instead change the require of index.js in /Users/shaun/projects/front-end-monorepo/packages/lib-classifier/src/components/Classifier/components/Feedback/components/Graph2dRangeFeedback.js to a dynamic import() which is available in all CommonJS modules.

and etc

OK, so upon review:

  • D3 now ships as pure ES modules and requires Node.js 12 or higher. - this IS a problem.

Additional reading:

Status

🤷 throw_hands_up_emoji

Good gravy, I need to check with the other devs to see if anybody else has encountered the ESM import issues. I might have missed something someone on the team is already aware of, since this doesn't seem to be a niche problem.

@eatyourgreens
Copy link
Contributor Author

Why our Mocha tests are using CommonJS to require modules, I do not know. Some Babel config rewriting import as require?

@eatyourgreens
Copy link
Contributor Author

We could try adding regenerator-runtime/runtime to the Mocha config.
https://stackoverflow.com/a/60522428

@eatyourgreens
Copy link
Contributor Author

We could also try publishing the libraries as ES Modules, but need to be mindful of not breaking Zoo Notes or Mapping Viz.

@eatyourgreens
Copy link
Contributor Author

Mocha Babel examples might be useful.

@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Nov 28, 2022

#3900 adds ES modules to @zooniverse/classifier and @zooniverse/react-components, alongside the UMD modules built by webpack. Builds and deploys use the ES modules. Mocha tests require the UMD modules. Everything works but we need webpack builds, which are pretty big and slow, in order to run the tests.

#3920 converts @zooniverse/async-states, which is a single JS file, from CommonJS to ES6. Builds that use it run fine, but tests fail for both NextJS apps and the classifier, when they try to require the ES module.

@eatyourgreens
Copy link
Contributor Author

#4064 removed the webpack UMD builds. The classifier and Zooniverse React Components are published as ESM, for the NextJS builds, and CommonJS for the Mocha tests.

@goplayoutside3
Copy link
Contributor

Going to close #3781 for now, but linking here because of relation to ESM

@eatyourgreens
Copy link
Contributor Author

@zooniverse/panoptes-js also uses require and module.exports (CommonJS.)

@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Sep 12, 2023

Shaun's ESM notes here are very useful.

@eatyourgreens
Copy link
Contributor Author

next-i18next breaks if you convert the config file to ES6.

There may be a workaround. See the conversation on that issue.

@eatyourgreens
Copy link
Contributor Author

next-i18next also breaks if you rename the config to next-i18next.config.cjs.

@eatyourgreens
Copy link
Contributor Author

Bun has a good explanation of Node module resolution and the differences between CommonJS and ESM.

https://bun.sh/docs/runtime/modules

@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Mar 19, 2024

Node 21 now has experimental support for require(esm).

https://joyeecheung.github.io/blog/2024/03/18/require-esm-in-node-js/

@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Oct 20, 2024

require(esm) is now default behaviour in Node 23. 🎉

https://nodejs.org/en/blog/release/v23.0.0

I think you can enable it with a flag in Node 22, which will enter LTS next week.

Top level await is still restricted to modules.

With this feature enabled, Node.js will no longer throw ERR_REQUIRE_ESM if require() is used to load a ES module. It can, however, throw ERR_REQUIRE_ASYNC_MODULE if the ES module being loaded or its dependencies contain top-level await. When the ES module is loaded successfully by require(), the returned object will be a ES module namespace object similar to what's returned by import(), and can be checked using util.isModuleNamespaceObject().

@eatyourgreens
Copy link
Contributor Author

How to convert CommonJS to ESM, just published yesterday.

@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Oct 25, 2024

If you switch to Node 22, then yarn bootstrap and yarn test both fail 😞. I discovered that by accident today, when I forgot to switch back from Node 22 to Node 20 while building the monorepo.

It also looks like module loading has changed in Node 22. This warning is from lib-content, but it applies to all the monorepo packages without type declarations:

@zooniverse/content:test
$ mocha --config ./test/.mocharc.json ./.storybook/specConfig.js "./src/**/*.spec.js"
      (node:69592) [MODULE_TYPELESS_PACKAGE_JSON] Warning: Module type of file:///Users/jimodonnell/zooniverse/front-end-monorepo/packages/lib-content/test/setup.js is not specified and it doesn't parse as CommonJS.
      Reparsing as ES module because module syntax was detected. This incurs a performance overhead.
      To eliminate this warning, add "type": "module" to /Users/jimodonnell/zooniverse/front-end-monorepo/packages/lib-content/package.json.
      (Use `node --trace-warnings ...` to show where the warning was created)

@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Oct 25, 2024

require(esm) is also available in Node 20.17, behind the --experimental-require-module flag. That should allow you to upgrade packages without having to upgrade Node.

https://nodejs.org/en/blog/release/v20.17.0

@goplayoutside3
Copy link
Contributor

goplayoutside3 commented Dec 6, 2024

I was investigating this mocha config x ESM issue yesterday and want steer this Github discussion toward potential solutions for the monorepo, ideally without completely change our testing architecture.

Please correct me if I'm wrong, but in summary:

  • FEM's mocha config uses @babel/register to transpile. The purpose of babel is to transpile your js code to an understandable version of js for the given tool you're using. Mocha does not understand JSX, so in FEM babel is a must.
  • Third-party package maintainers are moving toward ESM-only, which is fine for building and running the Next.js apps, or Storybook in the browser, but throws Error [ERR_REQUIRE_ESM]: require() of ES Module [third-party package] from [FEM component] not supported in a testing environment. For example: mime v4 in lib-content's unit tests.
  • Mocha can work with ESM-only. This requires type: module in the package.json.
    • I experimented with adding type: module to lib-content, ensuring no require() syntax in the library code, and even excluding its jsdom setup file. However, this config results in the problem documented here where I saw Unexpected token '<' thrown on the first .spec.js file. babel/register does not support compiling native Node.js ES modules on the fly (bottom of their docs here).
    • At this point, I'm not sure if this shortcoming of @babel/register is a complete blocker to FEM's architecture of React + Babel + Mocha.

@eatyourgreens you've left a few comments with potential solutions. Did you try any of these combos?

  • The --experimental-require-module flag with Node v20
  • Node v22 with type: module in a library?

@eatyourgreens
Copy link
Contributor Author

I just ran a very quick experiment with Node 22:

nvm use 22
yarn install --ignore-scripts
yarn bootstrap

The project app build fails with:

unhandledRejection TypeError: Cannot set properties of undefined (setting 'eio')
    at /Users/jimodonnell/zooniverse/front-end-monorepo/node_modules/panoptes-client/lib/SugarClient/primus.js:3805:9
    at Array.<anonymous> (/Users/jimodonnell/zooniverse/front-end-monorepo/node_modules/panoptes-client/lib/SugarClient/primus.js:3806:3)
    at UMDish (/Users/jimodonnell/zooniverse/front-end-monorepo/node_modules/panoptes-client/lib/SugarClient/primus.js:4:15)
    at Object.<anonymous> (/Users/jimodonnell/zooniverse/front-end-monorepo/node_modules/panoptes-client/lib/SugarClient/primus.js:11:3)
    at Module._compile (node:internal/modules/cjs/loader:1546:14)
    at Object..js (node:internal/modules/cjs/loader:1689:10)
    at Module.load (node:internal/modules/cjs/loader:1318:32)
    at Function._load (node:internal/modules/cjs/loader:1128:12)
    at TracingChannel.traceSync (node:diagnostics_channel:315:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:218:24) {
  type: 'TypeError'
}

@goplayoutside3
Copy link
Contributor

goplayoutside3 commented Dec 6, 2024

cannot set properties of undefined (setting 'eio')

This looks similar to the hexoid error previously thrown by app-root that was fixed by using alias in the webpack config object in next.config.js.

@eatyourgreens
Copy link
Contributor Author

One problem the monorepo has is that both JS and JSX files use the .js extension, so build tools would have to assume that every .js file is a JSX file and transpile to JS.

If you look at PFE, for example, I think JSX files use .jsx there. Changing all the file extensions for the monorepo doesn't seem practical, though.

@eatyourgreens
Copy link
Contributor Author

Oh, looks like I already tried Node 22 back in October: #3779 (comment)

@goplayoutside3
Copy link
Contributor

Oh, looks like I already tried Node 22 back in October:

Right, that's why I was curious if you'd tried Node v22 while also adjusting type: module in package.json files.

@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Dec 6, 2024

Adding type: module to lib-content, then running the tests, errors, because the tests set window.navigator, which is read-only.

@zooniverse/content:test
$ mocha --config ./test/.mocharc.json ./.storybook/specConfig.js "./src/**/*.spec.js"
      
       Exception during run: TypeError: Cannot set property navigator of #<Object> which has only a getter
          at file:///Users/jimodonnell/zooniverse/front-end-monorepo/packages/lib-content/test/setup.js:43:18
          at ModuleJob.run (node:internal/modules/esm/module_job:268:25)
          at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:543:26)
          at async formattedImport (/Users/jimodonnell/zooniverse/front-end-monorepo/node_modules/mocha/lib/nodejs/esm-utils.js:9:14)
          at async exports.requireOrImport (/Users/jimodonnell/zooniverse/front-end-monorepo/node_modules/mocha/lib/nodejs/esm-utils.js:42:28)
          at async exports.loadFilesAsync (/Users/jimodonnell/zooniverse/front-end-monorepo/node_modules/mocha/lib/nodejs/esm-utils.js:100:20)
          at async singleRun (/Users/jimodonnell/zooniverse/front-end-monorepo/node_modules/mocha/lib/cli/run-helpers.js:162:3)
          at async exports.handler (/Users/jimodonnell/zooniverse/front-end-monorepo/node_modules/mocha/lib/cli/run.js:375:5)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Modules run in strict mode, so that's probably the source of that error.

@eatyourgreens
Copy link
Contributor Author

Removing the offending line, then running the lib-content tests again, gets us back to unexpected token < because JSX isn't JS.

@zooniverse/content:test
$ mocha --config ./test/.mocharc.json ./.storybook/specConfig.js "./src/**/*.spec.js"
      
       Exception during run: SyntaxError[ @/Users/jimodonnell/zooniverse/front-end-monorepo/packages/lib-content/.storybook/specConfig.js ]: Unexpected token '<'
          at compileSourceTextModule (node:internal/modules/esm/utils:340:16)
          at ModuleLoader.moduleStrategy (node:internal/modules/esm/translators:102:18)
          at #translate (node:internal/modules/esm/loader:433:12)
          at ModuleLoader.loadAndTranslate (node:internal/modules/esm/loader:480:27)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Dec 6, 2024

Mocha tests containing JSX with ESM suggests running Mocha with a custom Node loader, but I don't know how old that is or which versions of Node it was written for.

@node-loader/babel looks like it's still maintained, so could be worth trying.

@eatyourgreens
Copy link
Contributor Author

I have to confess that currently I use Storybook with Playwright to run tests for JSX components in a pure ESM project, and it just works out-of-the-box.

https://github.com/nismod/irv-jamaica/blob/83a31ea2e4a20b4a7ab42555b262b4af8894dfcc/.github/workflows/storybook-tests.yml#L19-L29

@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Dec 6, 2024

I was investigating this mocha config x ESM issue yesterday and want steer this Github discussion toward potential solutions for the monorepo, ideally without completely change our testing architecture.

This seems like an excellent summary to me. I'd add a couple of points:

  • Older packages like lib-panoptes-js were written before Node supported import/export, so are still using require and module.exports. This leads to CI errors in Panoptes.js: add lib-panoptes-js dev server, add experimental auth #6375, where browser code (import/export) exists alongside older Node code(require/module exports). Now that Node supports import and export, it might be worth taking some time to remove require and module exports= from all packages. That might require converting them from type: commonjs to type: module.
  • Up until Node 20, type: commonjs is the default for any package that doesn't explicitly specify a type. The Node 22 module loader seems to dynamically determine a type, by inspecting a file for import/export syntax. This can lead to errors if a file uses ES6 syntax but doesn't run in strict mode. I'd suggest explicitly setting type: commonjs on all packages, until they've been converted to ESM. You can also test if files work in strict mode by adding the use strict directive at the top of a file.

@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Dec 7, 2024

This looks similar to the hexoid error previously thrown by app-root that was fixed by using alias in the webpack config object in next.config.js.

The Sugar client is crashing here, when it adds engine-io to the window (or to the Primus client instance).

https://github.com/zooniverse/panoptes-javascript-client/blob/721b696dccecf86a35614244ab29d19d1a62c3b7/lib/SugarClient/primus.js#L3795-L3806

The Sugar client, and in fact the Panoptes JS Client in general, only runs in browsers, but monorepo code runs in Node and in browsers. So that might be causing problems when window is undefined.

Something to be aware of in #6375 too. That PR is adding browser-only code to a library, @zooniverse/panoptes-js, that's designed to be isomorphic. I still recommend taking a look at the new, isomorphic Panoptes auth client that Roger started writing for the monorepo (#6376), if only to get an idea of what problems you might run into adapting auth to run in Next.js.

@eatyourgreens
Copy link
Contributor Author

#3920 converted lib-async-states to ESM. It might be worth revisiting that with Node 20 or Node 22.

@goplayoutside3
Copy link
Contributor

I've started to track the third-party libraries that cause tests to fail in the top level description of this Issue.

@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Dec 11, 2024

I tagged Dependabot PRs with ESM if they break the tests. I don’t know if that still happens. Labels are useful if you want to track a dynamic list of PRs.

https://github.com/zooniverse/front-end-monorepo/issues?q=label%3AESM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: To do
Development

No branches or pull requests

2 participants