fix: unable to cyclic ES modules

[takerusilt]

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]

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 [email protected]. Thanks!

[SimenB]

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

[facebook-github-bot]

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

[takerusilt]

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]

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]

Done, including the changelog update :)

[SimenB]

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

[SimenB]

thanks!

[github-actions]

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.