fix(runtime): refactor ESM code to avoid race condition#11150
fix(runtime): refactor ESM code to avoid race condition#11150SimenB merged 3 commits intojestjs:masterfrom
Conversation
SimenB
changed the title
link at correct time
SimenB
changed the title
| ); | ||
| } | ||
|
|
||
| await this._esmModuleLinkingMap.get(module); |
There was a problem hiding this comment.
could also choose to just do await if status is unlinked or linking probably. In other cases the promise will be resolved (or rejected) though, so I don't think it really matters
| await this._esmModuleLinkingMap.get(module); | ||
|
|
||
| if (module.status === 'linked') { | ||
| await module.evaluate(); |
There was a problem hiding this comment.
could do a weakmap here as well, but I think the if is enough and I don't think returning an evaluating module is an error? I.e. node will just wait for it to complete evaluation
| return this.loadEsmModule(modulePath, query); | ||
| const module = await this.loadEsmModule(modulePath, query); | ||
|
|
||
| return this.linkAndEvaluateModule(module); |
There was a problem hiding this comment.
bug fix is mostly here - only call linkAndEvaluateModule on the entry point, plus dynamic imports. The old loadEsmModule now just creates the module and sticks it in the cache
|
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. |
2 participants
Summary
This refactor makes sure to only call
linkandevaluateat the top level module or any dynamic imports. This resolves an issue where we produced an invalid module graph, leading newer versions of Node to throw an error.This also contains a revert of the implementation added in #10892, but keeps the test as the complexity there is not needed anymore with this more correct way of linking and evaluating.
Fixes #11093.
Test plan
All existing tests pass in addition to the bug report in #11093.