Skip to content

import-in-the-middle failing on Node.js main branch #995

Open

Description

@Trott

Ref: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3295/

 added 115 packages in 9s
 > import-in-the-middle@1.4.2 test
 > c8 --check-coverage --lines 85 imhotap --runner 'node test/runtest' --files test/{hook,low-level,other,get-esm-exports}/*
 TAP version 13
 1..25
 ok 1 test/get-esm-exports/v20-get-esm-exports.js
   ---
   stdout: >-
     export let name1, name2/*,  */; // also var
        contains exports: name1,name2
     export const name1 = 1, name2 = 2/*,  */; // also var, let
        contains exports: name1,name2
     export function functionName() { /*  */ }
        contains exports: functionName
     export class ClassName { /*  */ }
        contains exports: ClassName
     export function* generatorFunctionName() { /*  */ }
        contains exports: generatorFunctionName
     export const { name1, name2: bar } = o;
        contains exports: name1,bar
     export const [ name1, name2 ] = array;
        contains exports: name1,name2
     let name1, nameN; export { name1, /* , */ nameN };
        contains exports: name1,nameN
     let variable1, variable2, nameN; export { variable1 as name1, variable2 as
     name2, /* , */ nameN };
        contains exports: name1,name2,nameN
     let variable1; export { variable1 as "string name" };
        contains exports: string name
     let name1; export { name1 as default /*,  */ };
        contains exports: default
     export default expression;
        contains exports: default
     export default function functionName() { /*  */ }
        contains exports: default
     export default class ClassName { /*  */ }
        contains exports: default
     export default function* generatorFunctionName() { /*  */ }
        contains exports: default
     export default function () { /*  */ }
        contains exports: default
     export default class { /*  */ }
        contains exports: default
     export default function* () { /*  */ }
        contains exports: default
     export * from "module-name";
        contains exports: *
     export * as name1 from "module-name";
        contains exports: name1
     export { name1, /* , */ nameN } from "module-name";
        contains exports: name1,nameN
     export { import1 as name1, import2 as name2, /* , */ nameN } from
     "module-name";
        contains exports: name1,name2,nameN
     export { default, /* , */ } from "module-name";
        contains exports: default
     export { default as name1 } from "module-name";
        contains exports: name1
   stderr: ''
   ...
 ok 2 test/hook/define-property.js
 ok 3 test/hook/dynamic-import-default.js
 ok 4 test/hook/dynamic-import-default.mjs
 ok 5 test/hook/dynamic-import.js
 ok 6 test/hook/dynamic-import.mjs
 ok 7 test/hook/loader.mjs
 ok 8 test/hook/remove.mjs
 ok 9 test/hook/static-import-default.mjs
 ok 10 test/hook/static-import-disabled.mjs
 ok 11 test/hook/static-import-package-internals-enabled.mjs
 ok 12 test/hook/static-import-package-internals.mjs
 ok 13 test/hook/static-import-package.mjs
 ok 14 test/hook/static-import.mjs
 ok 15 test/hook/v18-static-import-assert.mjs
 ok 16 test/low-level/dynamic-import-default.js
 ok 17 test/low-level/dynamic-import-default.mjs
 ok 18 test/low-level/dynamic-import.js
 ok 19 test/low-level/remove.mjs
 ok 20 test/low-level/sanitized-url.mjs
 ok 21 test/low-level/static-import-default.mjs
 ok 22 test/low-level/static-import-disabled.mjs
 ok 23 test/low-level/static-import.mjs
 ok 24 test/other/executable
 not ok 25 test/other/import-executable.mjs
   ---
   stdout: ''
   stderr: |-
     node:internal/process/promises:262
               triggerUncaughtException(err, true /* fromPromise */);
               ^
     AssertionError [ERR_ASSERTION]: Missing expected rejection (TypeError).
         at async file:///home/iojs/tmp/citgm_tmp/8aff2e39-3a70-4ba0-832b-b7bb6f1d8719/import-in-the-middle/test/other/import-executable.mjs:7:3 {
       generatedMessage: false,
       code: 'ERR_ASSERTION',
       actual: undefined,
       expected: { name: 'TypeError', code: 'ERR_UNKNOWN_FILE_EXTENSION' },
       operator: 'rejects'
     }
     Node.js v21.0.0-pre
   ...

@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.

Activity

  1. merceyz commented on Oct 1, 2023

    @merceyz
    Member

    One of the Yarn tests also noticed this change and was mentioned on Slack, likely cause is nodejs/node#49869.

  2. bengl commented on Oct 1, 2023

    @bengl
    Member

    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 nodejs#main and I get the same test failure.

    I'll bisect to see where this was introduced.

    Appendix

    To reproduce this:

    1. Create a file called executable in some directory, with a Node.js shebang line (i.e. #!/usr/bin/env node), and make it executable.
    2. Put the following file in the same directory and run it with nodejs#main (or the commit from citgm-smoker), and compare that with 20.8.0:
    // import-executable.js
    import { rejects } from 'assert'
    rejects(() => import('./executable'), {
      name: 'TypeError',
      code: 'ERR_UNKNOWN_FILE_EXTENSION'
    })
  3. bengl commented on Oct 1, 2023

    @bengl
    Member

    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.

  4. GeoffreyBooth commented on Oct 1, 2023

    @GeoffreyBooth
    Member

    Based on my read of the PR, I think that might be an unintended consequence? cc @GeoffreyBooth

    Import of extensionless files is intended when --default-type=module is passed, but not otherwise. nodejs/node#49974 allows it without the flag, limited to module package scopes.

  5. GeoffreyBooth commented on Oct 2, 2023

    @GeoffreyBooth
    Member

    @bengl Actually I need to be more specific. The new flag was supposed to enable the import of extensionless ES modules. In normal node without using the new flag, import of extensionless CommonJS modules, which are any extensionless files not in a module scope, was always supposed to be possible. It was a bug that previously they were erroring.

    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 cjs-bin is a CommonJS file in a CommonJS scope. I wasn’t aware of this bug, but nodejs/node#49869 fixed it 😄

    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 main, node ./test.mjs errors. But run as node --experimental-default-type=module ./test.mjs, it succeeds.

  6. alfonsograziano commented on Oct 11, 2023

    @alfonsograziano
    Contributor

    @bengl @Qard hello :)
    Sorry for the ping, the module is constantly failing on all the CITGM run on Node v21.x
    Here the last run: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3318/

    Do you know how can we approach the problem to fix it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions