vercel / nft

Node.js dependency tracing utility
https://npmjs.com/@vercel/nft
MIT License
1.31k stars 136 forks source link

fix: Support resolving `module.register` dependencies #429

Closed timfish closed 1 month ago

timfish commented 3 months ago

Closes #428

module.register(specifier[, parentURL][, options])

timfish commented 3 months ago

Looks like I need to pull my Windows laptop out and work out why those tests are failing!

timfish commented 3 months ago

So the failing unit test (zeromq-node-gyp) passes in isolation on my Windows machine but fails when all the tests are run.

One thing I don't understand is how it's passing at all, even on other platforms.

The expected output is: https://github.com/vercel/nft/blob/31ab41706201cadf09763273199af2aebd6dfcc2/test/unit/zeromq-node-gyp/output.js#L1-L11

But it's failing with an extra file included:

        "node_modules\\@aminya\\node-gyp-build\\index.js",
    +   "node_modules\\@aminya\\node-gyp-build\\node-gyp-build.js",
        "node_modules\\@aminya\\node-gyp-build\\package.json",
        "node_modules\\zeromq\\lib\\draft.js",
        "node_modules\\zeromq\\lib\\index.js",
        "node_modules\\zeromq\\lib\\native.js",
        "node_modules\\zeromq\\lib\\util.js",

And the reason it's being included:

      'node_modules\\@aminya\\node-gyp-build\\node-gyp-build.js' => {
        type: [ 'dependency' ],
        ignored: false,
        parents: Set(1) { 'node_modules\\@aminya\\node-gyp-build\\index.js' }
      }

And then when you look at the actual source of the parent, it's obvious why this is being included:

} else { // else use the runtime version here
  module.exports = require('./node-gyp-build.js')
}

So how is this passing on every other platform but failing on Windows when the full test suite is run?

styfle commented 3 months ago

This might fix it: https://github.com/vercel/nft/pull/432

I merged it in to see if tests pass now 🤞

timfish commented 3 months ago

Unfortunatly not 😭

I'm more confused as to how this test passes on any platform. The additional dependency in the failing case looks like it should be included!

styfle commented 3 months ago

Looks like the only failing test is the new one now. Its missing hook.mjs, hook2.mjs and hook3.mjs.

 ● should correctly trace module-register from cwd

    expect(received).toEqual(expected) // deep equality

    - Expected  - 3
    + Received  + 0

      Array [
        "package.json",
    -   "test\\unit\\module-register\\hook.mjs",
    -   "test\\unit\\module-register\\hook2.mjs",
    -   "test\\unit\\module-register\\hook3.mjs",
        "test\\unit\\module-register\\input-esm.mjs",
        "test\\unit\\module-register\\input.js",
        "test\\unit\\module-register\\node_modules\\test-pkg\\index.mjs",
        "test\\unit\\module-register\\node_modules\\test-pkg\\package.json",
      ]

https://github.com/vercel/nft/actions/runs/9785332243/job/27018410588?pr=429#step:9:1063

timfish commented 3 months ago

On my Windows 11 machine the module-register test passes.

image

I probably need to revisit this at a more reasonable hour in the morning!

timfish commented 3 months ago

So I reverted the path.sep change and the tests now all pass on Windows.

Looking at the resulting deps and imports and on Windows for ALL the tests, these all contain forwards slashes /.

s1gr1d commented 2 months ago

@styfle can this be merged?

github-actions[bot] commented 1 month ago

:tada: This PR is included in version 0.27.4 :tada:

The release is available on:

Your semantic-release bot :package::rocket: