fix: always fallback to import() if require() fails #5384
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates the fallback mechanism when requiring modules by removing the conditional check for asynchronous modules and always falling back to importing.
- Removes specific error code check for ERR_REQUIRE_ASYNC_MODULE
- Always uses formattedImport on require failure in the esm-utils module
Comments suppressed due to low confidence (1)
lib/nodejs/esm-utils.js:96
- Changing the fallback to always use formattedImport on require failure may inadvertently mask errors that are not related to asynchronous modules. Please verify that this behavior aligns with the intended API design.
// Import if require fails.
Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
|
Seems to fix the error locally using node 22, could not reproduce the same error that happened in CI using node 24.. |
|
#5383's added testing passes with this change for me on Node.js 22.10.0, 22.12.0, and 24.2.0. I'm still testing out other issue repros, but if all goes well I'd say we merge this PR then, merge #5383 right after. Aside: #5376 tracks existing integration test failures. Those aren't something we need to address here. |
There was a problem hiding this comment.
This fix looks great to me, thank you! Confirmed locally that all 3 currently reported issues from users are resolved by this change.
Just thinking out load: it makes sense to me that the logic should fall back to the traditional import() style loading if require() fails. There isn't a comprehensive list anywhere of the various ways folks have hooked into module loading. This change has us now use the legacy behavior as a catch-all for legacy behavior, rather than one known case. 👍
|
I will check this out later today and see how it goes. |
|
Only reason not to do this would be to fail fast in cases we know will fail in both – and optimizing to fail fast feels like a very low priority. So I'm 👍👍 |
PR Checklist
ts-node/esm#5388status: accepting prsOverview
Current implementation only forwards to import if the error is ERR_REQUIRE_ASYNC_MODULE which is wrong since node loaders can do file name conversion like loading a
.tsfile instead of.jsfile.This PR always tries to import a module that failed to be required.
This implementation sounds more correct since old behavior was to always use import.
Related to #5366.