Don't compile import() in development
#12288
Conversation
nicolo-ribaudo
added
area: tests
PR: Internal 🏠
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/31498/ |
|
Oh no another segfault 🙃 |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit c12b413:
|
| function loadOptionsAsync({ filename, cwd = __dirname }, mjs) { | ||
| if (mjs) { | ||
| // import() crashes with jest | ||
| return loadOptionsAsyncInSpawedProcess({ filename, cwd }); |
There was a problem hiding this comment.
@SimenB This is needed to spawn tests using import() in a different process so that Jest doesn't have to handle them.
If you remove this line (and thus run the tests in the current Node.js process managed by Jest), and run node --experimental-vm-modules ./node_modules/.bin/jest babel-core/test/config-chain it crashes.
The file containing import() is https://github.com/babel/babel/blob/main/packages/babel-core/src/config/files/import.js, and you can Ctrl+F for "babel.config.mjs" in this file to find a test using import() internally.
I'll create a branch with a smaller test file to make it easier to isolate the problem (even if I'm not able to reproduce it in a fresh project containing only Jest).
There was a problem hiding this comment.
yeah, this is weird. The importModuleDynamically callback isn't invoked at all - I'm guessing this is some edge case in Node's implementation
There was a problem hiding this comment.
I think it will be better if I post the reproduction to the Node.js repo then: even if the tests or Jest did something wrong (but from my superficial debugging I don't think so), Node.js shouldn't crash.
There was a problem hiding this comment.
From debugging - putting a breakpoint on the import() expression shows it is called with an URL with the following href: file:///var/folders/gj/0mygpdfn6598xh34njlyrqzc0000gn/T/babel-test-load-config-async-babel.config.mjsYKe5KD/babel.config.mjs. This file exists in disk and is valid ESM. Then doing "Step over next function call" crashes with a segfault without stopping the debugger.
PID 43445 received SIGSEGV for address: 0x0
0 segfault-handler.node 0x00000001055e3fa0 _ZL16segfault_handleriP9__siginfoPv + 304
1 libsystem_platform.dylib 0x00007fff6951f5fd _sigtramp + 29
2 ??? 0x0000000000000007 0x0 + 7
3 node 0x000000010033df4a _ZN2v88internal7Isolate38RunHostImportModuleDynamicallyCallbackENS0_6HandleINS0_6ScriptEEENS2_INS0_6ObjectEEE + 138
4 node 0x00000001006f82f4 _ZN2v88internal25Runtime_DynamicImportCallEiPmPNS0_7IsolateE + 340
5 node 0x0000000100a797b4 Builtins_CEntry_Return1_DontSaveFPRegs_ArgvInRegister_NoBuiltinExit + 52
[1] 43445 segmentation fault node --experimental-vm-modules --inspect-brk ./node_modules/.bin/jest
There was a problem hiding this comment.
Ref: nodejs/node#35889
| // "minNodeVersion": "10.0.0" <-- For Ctrl+F when dropping node 10 | ||
| const supportsESM = parseInt(process.versions.node) >= 12; | ||
|
|
||
| const isMJS = file => file.slice(-4) === ".mjs"; |
There was a problem hiding this comment.
nit: String#endsWith is available since Node.js 4
| const isMJS = file => file.slice(-4) === ".mjs"; | |
| const isMJS = file => file.endsWith(".mjs"); |
There was a problem hiding this comment.
can use https://nodejs.org/api/path.html#path_path_extname_path as well, dunno if it's any better, tho
| ); | ||
| return true; | ||
| } | ||
| if (esm && process.platform === "win32") { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
github-actions
bot
added
the
outdated
With this PR, we:
import()to require in developmentimport()when it's supported, and skip those tests in older Node.js versionsI wanted to use the native Jest support for
import(), but I'm getting some segmentation faults so this PR doesn't remove theloadOptionsAsyncInSpawedProcesshack.