-
-
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
feat: requireAndTranspileModule support ESM #11232
feat: requireAndTranspileModule support ESM #11232
Conversation
83e9687
to
e574c98
Compare
ba420fc
to
ae4f650
Compare
Hi, does |
1010a4d
to
e5e9f65
Compare
No. |
Codecov Report
@@ Coverage Diff @@
## master #11232 +/- ##
==========================================
- Coverage 64.24% 64.22% -0.03%
==========================================
Files 308 308
Lines 13499 13504 +5
Branches 3288 3290 +2
==========================================
Hits 8673 8673
- Misses 4117 4122 +5
Partials 709 709
Continue to review full report at Codecov.
|
import testResult from '@jest/test-result'; | ||
|
||
const {createEmptyTestResult} = testResult; |
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 testResult from '@jest/test-result'; | |
const {createEmptyTestResult} = testResult; | |
import {createEmptyTestResult} from '@jest/test-result'; |
does this work?
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.
No.
Because ts will compile to common js modules, and export using Object.defineProperty
with getter.
ESM named import from CJS module only support exports.name = value
way. esm commonjs namespaces
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.
Tested. It worked.
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.
That looks good to me
@WeiAnAn thanks, this looks really good! We've gotten a few PRs from first time contributors adding support for some which overlap with some you've added here. I think I'm gonna merge those even though this PR handles it a bit cleaner, then we'll need to resolve conflicts here |
Hey @WeiAnAn! Finally landed the other open PRs, so if you could rebase this that'd be awesome 👍 |
fccb777
to
02a697a
Compare
Codecov Report
@@ Coverage Diff @@
## master #11232 +/- ##
==========================================
+ Coverage 64.10% 64.18% +0.07%
==========================================
Files 308 308
Lines 13532 13516 -16
Branches 3297 3293 -4
==========================================
Hits 8675 8675
+ Misses 4142 4126 -16
Partials 715 715
Continue to review full report at Codecov.
|
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! I think we can just trim the changelog a bit, and remove a test which is already covered by othe rtests
Co-authored-by: Simen Bekkhus <[email protected]>
Co-authored-by: Simen Bekkhus <[email protected]>
@@ -27,6 +27,7 @@ const customTransformDIR = path.join( | |||
const nodeModulesDIR = path.join(tmpdir(), 'jest-global-setup-node-modules'); | |||
const rejectionDir = path.join(tmpdir(), 'jest-global-setup-rejection'); | |||
const e2eDir = path.resolve(__dirname, '../global-setup'); | |||
const esmTmpDir = path.join(tmpdir(), 'jest-global-setup-esm'); |
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.
is this (and the one in teardown) used?
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, the directory is created in setup file.
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.
sure, but what is that directory used for? In the test it looks like it's created and deleted but never used
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.
GlobalSetup/Teardown will read this directory and do some assertion.
If we don't clean directory before testing, it will failed in second round.
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.
even after 884c7e0
(#11232)?
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.
It fix #11267 globalSetup/globalTeardown e2e test.
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.
Aha, gotcha!
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
#11167
requireAndTranspileModule
is called by importingglobalSetup
,globalTeardown
,testRunner
,runner
,testEnvironment
andcreateTranspilingRequire
function.Only
createTranspilingRequire
may need to import none default exporter, so I refactorrequireAndTranspileModule
, which now acceptapplyInteropRequireDefault
option (defaulttrue
) to determine return module itself or default. So doesrequireAndTranspileModule
.Breaking Change
requireAndTranspileModule
always return a Promise.createTranspilingRequire
return function which return a Promise now.Test plan
TODO