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

Jest configuration doesn't support pure-ESM packages #1000

Closed
karanbirsingh opened this issue Jan 3, 2022 · 4 comments
Closed

Jest configuration doesn't support pure-ESM packages #1000

karanbirsingh opened this issue Jan 3, 2022 · 4 comments
Labels
category: engineering status: resolved This issue has been merged into main and deployed to canary.

Comments

@karanbirsingh
Copy link
Contributor

karanbirsingh commented Jan 3, 2022

We've recently seen some dependabot updates to packages that are pure-ESM. This issue lists updates whose builds failed & whose update path wasn't immediately obvious - I didn't want to lose track of them. Looking closer & updating this description as we go

At first I thought this was because this repo targets Common JS; in general, trying to import ESM packages into CommonJS requires dynamic await() imports. I thought we'd need to follow the steps listed here: https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c

After trying the update locally, I noticed webpack overrides the require function so that overall index.js is still functional. The PR build failures come from jest tests with the error signature:

@accessibility-insights-action/shared: FAIL @accessibility-insights-action/shared src/ioc/setup-ioc-container.spec.ts
@accessibility-insights-action/shared:   ● Test suite failed to run
@accessibility-insights-action/shared:     Jest encountered an unexpected token
@accessibility-insights-action/shared:     Jest failed to parse a file. This happens e.g. when your code or its dependencies use non-standard JavaScript syntax, or when Jest is not configured to support such syntax.
@accessibility-insights-action/shared:     Out of the box Jest supports Babel, which will be used to transform your files into valid JS based on your Babel configuration.
@accessibility-insights-action/shared:     By default "node_modules" folder is ignored by transformers.
@accessibility-insights-action/shared:     Here's what you can do:
@accessibility-insights-action/shared:      • If you are trying to use ECMAScript Modules, see https://jestjs.io/docs/ecmascript-modules for how to enable it.
@accessibility-insights-action/shared:      • If you are trying to use TypeScript, see https://jestjs.io/docs/getting-started#using-typescript
@accessibility-insights-action/shared:      • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
@accessibility-insights-action/shared:      • If you need a custom transformation specify a "transform" option in your config.
@accessibility-insights-action/shared:      • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.
@accessibility-insights-action/shared:     You'll find more details and examples of these config options in the docs:
@accessibility-insights-action/shared:     https://jestjs.io/docs/configuration
@accessibility-insights-action/shared:     For information about custom transformations, see:
@accessibility-insights-action/shared:     https://jestjs.io/docs/code-transformation
@accessibility-insights-action/shared:     Details:
@accessibility-insights-action/shared:     /home/runner/work/accessibility-insights-action/accessibility-insights-action/node_modules/get-port/index.js:1
@accessibility-insights-action/shared:     ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,jest){import net from 'node:net';
@accessibility-insights-action/shared:                                                                                       ^^^^^^

Jest has experimental support for ECMAScript modules. We can try changing its configuration to unblock these PRs. We can also choose to defer the PRs. We have a little more leeway since we aren't exporting a library of our own.

@ghost ghost added the status: new This issue is new and requires triage by DRI. label Jan 3, 2022
@ghost ghost assigned lisli1 Jan 3, 2022
@karanbirsingh karanbirsingh added the status: ready for work This issue is ready to be worked on. label Jan 3, 2022
@ghost ghost removed the status: new This issue is new and requires triage by DRI. label Jan 3, 2022
@karanbirsingh karanbirsingh changed the title Current action implementation doesn't support pure-ESM packages Jest configuration doesn't support pure-ESM packages Jan 5, 2022
@katydecorah
Copy link
Contributor

I started looking into this and so far found two potential options that will allow this repository to accept pure ESM dependencies with Jest:

  1. Introduce babel to transform ESM modules. Adds new dependencies and configurations to maintain.
  2. Update tsconfig to allowJs and transform ESM modules. No new dependencies, but as I'm still learning the architecture of this repository, I'm not sure if the change to tsconfig will have any adverse effects.

@katydecorah
Copy link
Contributor

During triage, the team agreed that the second option (updating tsconfig) is the better option as it has lower overhead. This can move forward once the Node 16 work is in.

@katydecorah katydecorah added status: active This issue is currently being worked on by someone. and removed status: ready for work This issue is ready to be worked on. labels Aug 9, 2022
@katydecorah katydecorah self-assigned this Aug 9, 2022
@katydecorah
Copy link
Contributor

but as I'm still learning the architecture of this repository, I'm not sure if the change to tsconfig will have any adverse effects.

In case it's helpful for others: I ended up running yarn && yarn cbuild on main and ran git add -f to each package's dist folder. Then I switched over to branch with the tsconfig updates and ran yarn && yarn cbuild again. This gave me a local diff to see how the change to tsconfig affects the build packages (dist). This was very instructive as I learned I needed to update another setting in shared's tsconfig (to include only the src directory to prevent extraneous js files from change the package directory structure).

@katydecorah katydecorah added status: resolved This issue has been merged into main and deployed to canary. and removed status: active This issue is currently being worked on by someone. labels Aug 16, 2022
@katydecorah
Copy link
Contributor

This was resolved in #1268

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: engineering status: resolved This issue has been merged into main and deployed to canary.
Projects
None yet
Development

No branches or pull requests

3 participants