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

fix: unable to cyclic ES modules #10892

Merged
merged 4 commits into from Dec 1, 2020
Merged

fix: unable to cyclic ES modules #10892

merged 4 commits into from Dec 1, 2020

Conversation

takerusilt
Copy link
Contributor

Summary

In current implementation, when importing a mutually dependent ES module, stack overflow and promise deadlock occur.

For a circular dependency like follow:

moduleA.js:
import moduleB from './moduleB.js'; export default {};

moduleB.js:
import moduleA from './moduleA.js'; export default {};

Loading moduleA.js will trigger loadEsmModule of moduleB.js recursively from module.link before the promise for moduleA.js is set to _esmoduleRegistry; which in turn will recursively trigger loadEsmModule of moduleA.js, having a cache miss. Thus this will continue until stack overflow.

Another issue is that since the current cache registry is containing a promise that is resolved after the module is evaluated.
If there are circular dependencies, the promise in registry will never resolves because module evaluation is only done when all promises acquired through module.link are resolved.

Promise of moduleA.js is waiting for promise moduleB.js to resolve while promise of moduleB.js is also waiting for promise of moduleA.js to resolve.

Test plan

Test with moduleA and moduleB above.

@facebook-github-bot
Copy link
Contributor

Hi @takerusilt!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

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! This is looking really good. Very happy with somebody besides me messing with this code 👍

Could you add a test that fails without your changes? The current tests we have run from https://github.com/facebook/jest/blob/3cd04a50d82be392879202e5aefd16e1198ab12f/e2e/__tests__/nativeEsm.test.ts. Not sure if expanding our current test (native-esm.test.js) or adding some specific new circular ones makes sense. Your example from the OP seems like a nice test to add

CHANGELOG.md Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@takerusilt
Copy link
Contributor Author

Thanks! This is looking really good. Very happy with somebody besides me messing with this code 👍

Could you add a test that fails without your changes? The current tests we have run from https://github.com/facebook/jest/blob/3cd04a50d82be392879202e5aefd16e1198ab12f/e2e/__tests__/nativeEsm.test.ts. Not sure if expanding our current test (native-esm.test.js) or adding some specific new circular ones makes sense. Your example from the OP seems like a nice test to add

Thanks for pointing out where to add the test since I'm new to this repo :)
I have added the test and tried both without the changes (failed) and with the changes (passed).

@SimenB
Copy link
Member

SimenB commented Dec 1, 2020

Thanks! Can you run yarn jest e2e/__tests__/nativeEsm.test.ts -u to update the snapshot and fix the linting errors? We should be good to go after that 👍

@takerusilt
Copy link
Contributor Author

Done, including the changelog update :)

@SimenB
Copy link
Member

SimenB commented Dec 1, 2020

yarn check-copyright-headers is failing as well. Just copy from some other file 🙂

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!

@SimenB SimenB changed the title fix: unable to import mutually dependent ES module fix: unable to cyclic ES modules Dec 1, 2020
@SimenB SimenB merged commit 3559c3a into jestjs:master Dec 1, 2020
@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.

None yet

3 participants