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

feat: Jest ESM support #3256

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

johncrim
Copy link

@johncrim johncrim commented Feb 24, 2022

Note up front: I don't believe this is quite ready to merge, open issues/questions noted below. I'm posting this PR for initial review/evaluation.

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Unit tests (npm test) were run locally and passed
  • E2E Tests (npm run test.karma.prod) were run locally and passed
  • Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

GitHub Issue Number: #3230

What is the new behavior?

Jest tests are run using ES modules if the following changes are made:

  1. Execute node with --experimental-vm-modules, e.g. node --experimental-vm-modules node_modules/jest/bin/jest.js or NODE_OPTIONS=--experimental-vm-modules npx jest etc.
    I also like to include --no-warnings to eliminate the experimental feature warning on startup.
  2. Add to Stencil config: testing.useESModules: true

Alternatively, if jest is run directly (not through the stencil CLI), the following jest.config.cjs can be used to run stencil tests using ES modules:

module.exports = {
  preset: '@stencil/core/testing',

  globals: {
    stencil: {
      testing: {
        useESModules: true
      }
    }
  },
  extensionsToTreatAsEsm: ['.ts', '.tsx', '.jsx'],
};

In addition, a bug is fixed: If --runInBand is passed in the CLI, --max-workers=${config.maxConcurrentWorkers} is no longer set. I had to fix this in order to debug my work on this feature. The behavior was incorrect before (one of --runInBand or --max-workers wins, b/c --runInBand means no external worker processes), and I added tests to validate it.

I can break that out if you really want me to...

Does this introduce a breaking change?

  • Yes
  • No

Changes should not be breaking - the previous code paths are effectively unchanged, so Jest tests using CJS continue to run as before.

Testing

I added some new unit tests, which only validate the config generation. I also added a new test sub-project test/jest-esm. To run it:

npm run build
cd test/jest-esm
npm run jest ##Runs jest via the jest.config.cjs
npm run jest -- --clear-cache
npm run test ## Runs tests via stencil CLI

I didn't see a way to loop this project into the CI build; eg the test/jest-spec-runner isn't called in the github workflow.

Open issues

  1. Documentation. I added js docs to the new config values, but that's probably not sufficient. I didn't see an obvious place to add docs.

  2. More tests would be nice - eg running some jest + jest ESM tests through the whole stack, but I didn't see a pattern to follow.

  3. Jest expect extensions are currently borken when using ESM and @jest/globals. That's why there's this ugly expect:

(expect(page.root) as unknown as jest.JestMatchers<HTMLElement>).toEqualHtml

TLDR: The return type of @jest/globals expect() is not the same as jest.JestMatchers<T>. So anyone using Jest ESM with expect extensions will have to work around the typing issue until this issue is fixed. Note that this applies to anyone using Jest ESM, it is not stencil specific, but stencil developers likely use expect extensions.

I think the last issue is tolerable (and looks like it should be fixed soon) - since this is opt-in, I think it's reasonable to provide this as a preview feature while Jest improves their ESM support.

Other

I validated that these changes provide a fix for #3251, if projects affected by #3251 are willing to switch to using ES modules while testing. Since #3251 only appears due to ESM dependencies, I would think this would be attractive.

@johncrim johncrim requested a review from a team February 24, 2022 22:17
@johncrim
Copy link
Author

I added logging to help if useESModules is true, but node --experimental-vm-modules was not set:

❯ stencil test --spec
[31:01.5]  @stencil/core
[31:01.8]  [LOCAL DEV]
[31:01.9]  testing spec files
[31:02.0]  jest args: --spec --max-workers=8

[ ERROR ]  node must be run with --experimental-vm-modules for stencil.config testing.useESModules: true

... other errors

@rwaskiewicz rwaskiewicz added the Resolution: Refine This PR is marked for Jira refinement. We're not working on it - we're talking it through. label Mar 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Refine This PR is marked for Jira refinement. We're not working on it - we're talking it through.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants