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: experimental ES Modules support #9772

Merged
merged 10 commits into from
Apr 16, 2020
Merged

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Apr 5, 2020

Summary

This adds very basic support for running tests as native ES modules. 2 main limitations:

  1. it does not support mixing ESM and CJS
  2. the jest object on import.meta is an empty object. (meaning none of jest.* functions are available)

Another limitation is that the APIs we need only exist on node 13, and they're flagged as experimental

Like V8 test coverage, it only supports the node env, but using JSDOM 16 should also work, although I haven't bothered to verify.

Ref #9430.

Test plan

Integration test added. More of a smoketest than anything else.

@SimenB SimenB force-pushed the esm-experiments branch 3 times, most recently from 26037fb to 08fd412 Compare April 10, 2020 09:15
@codecov-io
Copy link

codecov-io commented Apr 10, 2020

Codecov Report

Merging #9772 into master will decrease coverage by 0.37%.
The diff coverage is 18.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9772      +/-   ##
==========================================
- Coverage   64.88%   64.51%   -0.38%     
==========================================
  Files         289      290       +1     
  Lines       12229    12318      +89     
  Branches     3030     3044      +14     
==========================================
+ Hits         7935     7947      +12     
- Misses       3654     3731      +77     
  Partials      640      640              
Impacted Files Coverage Δ
...circus/src/legacy-code-todo-rewrite/jestAdapter.ts 0.00% <0.00%> (ø)
packages/jest-jasmine2/src/index.ts 0.00% <0.00%> (ø)
packages/jest-runner/src/runTest.ts 2.12% <0.00%> (-0.10%) ⬇️
packages/jest-resolve/src/shouldLoadAsEsm.ts 10.00% <10.00%> (ø)
packages/jest-runtime/src/index.ts 61.29% <31.25%> (-3.39%) ⬇️
packages/jest-resolve/src/index.ts 44.78% <50.00%> (+0.06%) ⬆️

Continue to review full report at Codecov.

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

@SimenB SimenB force-pushed the esm-experiments branch 4 times, most recently from 1172f4d to 31e567b Compare April 12, 2020 17:45
Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤯 niceeeee

e2e/__tests__/nativeEsm.test.ts Outdated Show resolved Hide resolved
return false;
}

// this isn't correct - we might wanna load any file as a module (using synthetic module)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the issue with falling back to pkg.packageJson.type === 'module'; for non-JS as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly because it's not how node would do it - it expects explicit loaders. This is wrong, but I think less wrong. This whole thing is gonna be implemented in resolve at some point regardless

packages/jest-resolve/src/shouldLoadAsEsm.ts Show resolved Hide resolved
packages/jest-runtime/src/index.ts Outdated Show resolved Hide resolved
initializeImportMeta(meta: JestImportMeta) {
meta.url = pathToFileURL(modulePath).href;
// @ts-ignore TODO: fill this
meta.jest = {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to provide Jest in this way at all, or do we want to rely on import from '@jest/globals' for ESM instead?

Copy link
Member Author

@SimenB SimenB Apr 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure. The @jest/globals thing is a thing I came of with this morning. I think I like importing more than adding to meta (it's easier to type (no messing with the global ImportMeta interface) which usually means it's easier to consume as well), so maybe just drop it?

});
},
// should identifier be `node://${moduleName}`?
{context, identifier: moduleName},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally stack traces will show e.g. fs.js as the identifier. Have you tried a fs.readFileSync('/does/not/exist') to see what shows up with this logic?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't seem to show up at all

$ node --experimental-vm-modules file.mjs
(node:93098) ExperimentalWarning: The ESM module loader is experimental.
Error: ENOENT: no such file or directory, open '/does/not/exist'
    at Object.openSync (fs.js:462:3)
    at Module.readFileSync (fs.js:364:35)
    at myTestingModule.js:3:6
    at SourceTextModule.evaluate (internal/vm/module.js:221:32)
    at file:///Users/simen/repos/jest/file.mjs:35:22 {
  errno: -2,
  syscall: 'open',
  code: 'ENOENT',
  path: '/does/not/exist'
}
(node:93098) ExperimentalWarning: VM Modules is an experimental feature. This feature could change at any time

Test:

import {SourceTextModule, SyntheticModule} from 'vm';
import {createRequire} from 'module';

const require = createRequire(import.meta.url);
const fs = require('fs');

const required = fs;

const module = new SourceTextModule(
  `
  import * as fs from 'fs';
  fs.readFileSync('/does/not/exist');
`,
  {identifier: 'myTestingModule.js'},
);

module
  .link(async () => {
    const m = new SyntheticModule(
      ['default', ...Object.keys(required)],
      function () {
        this.setExport('default', required);
        Object.entries(required).forEach(([key, value]) => {
          this.setExport(key, value);
        });
      },
      {identifier: 'fs halla'},
    );

    await m.link(() => {});
    await m.evaluate();

    return m;
  })
  .then(() => module.evaluate())
  .catch(error => {
    process.exitCode = 1;

    console.error(error);
  });

// unstable as it should be replaced by https://github.com/nodejs/modules/issues/393, and we don't want people to use it
unstable_shouldLoadAsEsm = Resolver.unstable_shouldLoadAsEsm;

private async loadEsmModule(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another limitation you did not mention seems to be that it does not use any of the logic we have for normal modules regarding mocks, isolation, etc., correct? If so, we could maybe leave a comment saying this needs to be rewritten completely to share code with requireModule

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mocks are not implemented no, but isolation is. THe runtime itself is per test, so isolation should be correct. will need to implement mocks, though

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry that was unclear, I meant isolateModules

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aha, gotcha. isolateModules doesn't work with ESM regardless since it either has to be static or async. We probably need to do something else clever

@SimenB
Copy link
Member Author

SimenB commented Apr 16, 2020

I'll document this when it's more stable

@SimenB SimenB merged commit aa64672 into jestjs:master Apr 16, 2020
@SimenB SimenB deleted the esm-experiments branch April 16, 2020 19:59
@SimenB SimenB mentioned this pull request Apr 16, 2020
21 tasks
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants