-
Notifications
You must be signed in to change notification settings - Fork 148
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
import-in-the-middle failing on Node.js main branch #995
Comments
One of the Yarn tests also noticed this change and was mentioned on Slack, likely cause is nodejs/node#49869. |
The failing test ensures that IITM preserves a behaviour of Node.js core, when IITM's loader hooks are enabled. I believe it does, in fact, still preserve the behaviour of Node.js core, but that behaviour has changed, since I tested this on I'll bisect to see where this was introduced. AppendixTo reproduce this:
// import-executable.js
import { rejects } from 'assert'
rejects(() => import('./executable'), {
name: 'TypeError',
code: 'ERR_UNKNOWN_FILE_EXTENSION'
}) |
I hadn't seen the comment from @merceyz prior to bisecting, but I did arrive at the same PR, nodejs/node#49869. This PR seems to have introduced the ability to import extensionless executables. Based on my read of the PR, I think that might be an unintended consequence? cc @GeoffreyBooth We can skip the test in IITM for Node.js 21+ if that's intended. |
Import of extensionless files is intended when |
@bengl Actually I need to be more specific. The new flag was supposed to enable the import of extensionless ES modules. In normal So the example from Slack: printf "import bin from './cjs-bin';\nconsole.log(bin)" >test.mjs
printf "module.exports = {foo: 'bar'}" >cjs-bin
./node ./test.mjs May not have worked before nodejs/node#49869, but it should have, because Here’s a different example: mkdir package && cd package
printf '{"type": "module"}' > package.json
printf "import bin from './esm-bin';\nconsole.log(bin)" > test.mjs
printf "export default 'foo'" > esm-bin In latest |
@bengl @Qard hello :) Do you know how can we approach the problem to fix it? |
Ref: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3295/
@bengl @Qard Not sure if this is an expected breaking change in the main branch or a CITGM problem or what, but since it is failing on all platforms, I figure I better get attention on it sooner rather than later.
The text was updated successfully, but these errors were encountered: