fix(esm): dont bail for preflight requests#39237
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| } | ||
|
|
||
| const code = fs.readFileSync(filename, 'utf-8'); | ||
| const transformed = transformHook(code, filename, moduleUrl); |
There was a problem hiding this comment.
I'd skip this for preflights that we want to bail on, e.g. from node_modules. Since we are not supposed to transform them, we don't need to prepopulate source mapes.
I'd make a custom defaultLoad function that would return void 0 for preflights or call the actual defaultLoad for non-preflights.
There was a problem hiding this comment.
I didn't like the custom defaultLoad function, but I found a different way of writing it that skips the fs ops and transform for preflights, and looks pretty readable.
This comment has been minimized.
This comment has been minimized.
Skn0tt
force-pushed
the
dont-bail-for-preflight
branch
from
7fafc93 to
be4648f
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| moduleUrl = moduleUrl.replace(esmPreflightExtension, ''); | ||
| const filename = url.fileURLToPath(moduleUrl); | ||
| const format = kSupportedFormats.get(context.format) || (fileIsModule(filename) ? 'module' : 'commonjs'); | ||
| if (isPreflight) |
There was a problem hiding this comment.
This always skips transformHook which is the sole purpose of having a preflight.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| // Node < 18.6: defaultLoad takes 3 arguments. | ||
| // Node >= 18.6: nextLoad from the chain takes 2 arguments. | ||
| async function load(originalModuleUrl: string, context: { format?: string }, defaultLoad: Function) { | ||
| const isPreflight = originalModuleUrl.endsWith(esmPreflightExtension); |
There was a problem hiding this comment.
Let me offer an alternative as I think this change is somewhat missing the point. The like 65 in the old code is offender, it needs to go.
There was a problem hiding this comment.
There was a problem hiding this comment.
That patch let's through .esm.preflight loads to the next loader, which will fail because it can't find the file. It doesn't pass my reproduction test, and doesn't fix the issue.
There was a problem hiding this comment.
As discussed offline, I don't think we have a choice - 'bad' loader can be before us up in the chain.
There was a problem hiding this comment.
i updated the PR to be based on your patch, and i've added the catch for the bad loader and two more tests, take a look.
Skn0tt
force-pushed
the
dont-bail-for-preflight
branch
from
fb25b36 to
f5e4535
Compare
This comment has been minimized.
This comment has been minimized.
| // Bail for node_modules. | ||
| if (!shouldTransform(filename)) | ||
| if (!shouldTransform(filename)) { | ||
| if (isPreflight) |
There was a problem hiding this comment.
This violates the 'assume any order' consideration.
|
I verified that it still fixes #39172 - the |
Test results for "tests 1"4 failed 4 flaky38591 passed, 843 skipped Merge workflow run. |
Test results for "MCP"5 failed 5115 passed, 171 skipped Merge workflow run. |
3 participants
Closes #39172.
Interestingly, the test without the fix immediately leads to the
Error: duplicate test title "another test", first declared in example.spec.ts.esm.preflight:13bug, so we have a good repro even without tsx. It seems to affect any loader.