-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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: isolate esm async import bug #14397
Conversation
✅ Deploy Preview for jestjs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Looks right, but the Test Plan went missing (; Is it possible to add an e2e test? Hm.. As I was mentioning in the issue, there is no need to have jest/e2e/__tests__/resolveAsync.test.ts Lines 11 to 14 in 0fd5b1c
Or there is some reason, which I didn’t take into account? |
Sorry, I don't remember it. I'll fix them. |
I added test plan. But the |
I updated the test plan and the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! It is looking good.
I checked out and tried out on different versions of Node locally. Could you push the suggested changes to see if that also works in CI?
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
import {jest} from '@jest/globals'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import {jest} from '@jest/globals'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This import could not remove, because the next code will use the api jest. isolateModulesAsync
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Seems like CI is happy.
One setup detail could be polished. I left an inline comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was going through again and notice few more details. Could you look at them, please?
packages/jest-runtime/src/index.ts
Outdated
const registry = this._isolatedESMModuleRegistry | ||
? this._isolatedESMModuleRegistry | ||
: this._esmoduleRegistry; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm.. Isn’t it enough to use the same _isolatedMockRegistry
? Did you try that?
It was my idea about missing _isolatedESMModuleRegistry
, but it can I was wrong. Here we can see that _isolatedMockRegistry
was simply not used while loading ES modules. Sorry about slow thinking. Could you try to _isolatedMockRegistry
everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, it is possible. I originally thought it was intentionally differentiated because I saw that there was already an existing _isolatedModuleRegistry
, moreover, I think it seems more appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Simple and clear. Thanks for collaboration!
(CI fails are not related.)
can this pr be merged, if no other problems ? cc @SimenB @mrazauskas |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wonderful, thanks!
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. |
Summary
Fix: #14387
Test plan
test locally