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-util): add requireOrImportModule util for importing CJS or ESM #11199

Merged
merged 8 commits into from Mar 16, 2021

Conversation

WeiAnAn
Copy link
Contributor

@WeiAnAn WeiAnAn commented Mar 14, 2021

Summary

According to #11167, we will support ESM of pluggable modules.
We may use the same logic to import them, so I extract it into an util function.
I think it should be useful in later work!

Cons:
Will lose information of which module type importing in error message

- `Jest: Failed to load ESM transformer at ${transformPath} - did you use a default export?` 
+ `Jest: Failed to load ESM at ${filePath} - did you use a default export?`

But I think it is ok, user can find out by file path.

Test plan

  • Maybe an e2e test

TODO

  • Testing
  • Changelog

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Good idea! Can you update all usage of import after require fails to use this?

packages/jest-util/src/importModule.ts Outdated Show resolved Hide resolved
packages/jest-util/src/importModule.ts Outdated Show resolved Hide resolved
@WeiAnAn
Copy link
Contributor Author

WeiAnAn commented Mar 15, 2021

Good idea! Can you update all usage of import after require fails to use this?

Sure, in this pr or open another one?

@SimenB
Copy link
Member

SimenB commented Mar 15, 2021

This one I think - then we "prove" the abstraction works

Comment on lines -56 to -60
if (innerError.message === 'Not supported') {
throw new Error(
`Jest: Your version of Node does not support dynamic import - please enable it or use a .cjs file extension for file ${configPath}`,
);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All LTS Node should support dynamic import after Node 10 EOL.

Copy link
Member

Choose a reason for hiding this comment

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

Yep. Jest 27 will support Node 10, but 28 will drop it

Copy link
Member

Choose a reason for hiding this comment

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

so we should keep the warning for now

@WeiAnAn WeiAnAn changed the title feat(jest-util): add importModule util for importing CJS or ESM feat(jest-util): add requireOrImportModule util for importing CJS or ESM Mar 15, 2021
@WeiAnAn WeiAnAn requested a review from SimenB March 15, 2021 17:40
@WeiAnAn
Copy link
Contributor Author

WeiAnAn commented Mar 16, 2021

I encounter some problem when writing e2e test.
In the ESM mode, using requireOrImportModule got Jest encountered an unexpected token error, although I already set transform: {}, with --experimental-vm-modules node options, and add "type": "module" in package.json.
I will try to figure out this.

@SimenB
Copy link
Member

SimenB commented Mar 16, 2021

You don't have to write a new e2e test for this, the existing e2e tests for the modules that are written in ESM are enough as long as they use this new util 🙂

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

thanks!

@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 10, 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.

None yet

3 participants