Skip to content

Comments

fix: Support resolving module.register dependencies#429

Merged
kodiakhq[bot] merged 16 commits intovercel:mainfrom
timfish:fix/module-register
Sep 3, 2024
Merged

fix: Support resolving module.register dependencies#429
kodiakhq[bot] merged 16 commits intovercel:mainfrom
timfish:fix/module-register

Conversation

@timfish
Copy link
Contributor

@timfish timfish commented Jun 25, 2024 •
edited
Loading

@timfish timfish requested review from ijjk and styfle as code owners June 25, 2024 21:32
styfle
styfle reviewed Jun 26, 2024
View reviewed changes
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

Can you change the integration test into a unit test instead?

That way we can see see the input and output to make sure these remain the same instead of relying on implementation details of a 3rd party package.

styfle
styfle reviewed Jun 26, 2024
View reviewed changes
styfle
styfle reviewed Jun 27, 2024
View reviewed changes
styfle
styfle reviewed Jun 27, 2024
View reviewed changes
@timfish timfish requested a review from styfle June 27, 2024 17:36
styfle
styfle reviewed Jun 28, 2024
View reviewed changes
@timfish
Copy link
Contributor Author

timfish commented Jul 3, 2024

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

timfish
timfish commented Jul 3, 2024
View reviewed changes
@timfish
Copy link
Contributor Author

timfish commented Jul 3, 2024

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:

nft/test/unit/zeromq-node-gyp/output.js

Lines 1 to 11 in 31ab417

[
"node_modules/@aminya/node-gyp-build/index.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",
"node_modules/zeromq/package.json",
"package.json",
"test/unit/zeromq-node-gyp/input.js"
]

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 styfle requested a review from a team as a code owner July 3, 2024 21:35
@styfle
Copy link
Member

styfle commented Jul 3, 2024

This might fix it: #432

I merged it in to see if tests pass now 🤞

@timfish
Copy link
Contributor Author

timfish commented Jul 3, 2024

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
Copy link
Member

styfle commented Jul 3, 2024 •
edited
Loading

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

This comment was marked as outdated.

Sign in to view
@timfish timfish force-pushed the fix/module-register branch from 6e1d464 to 5cd460d Compare July 4, 2024 09:30
@timfish
Copy link
Contributor Author

timfish commented Jul 4, 2024 •
edited
Loading

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

@timfish timfish mentioned this pull request Jul 10, 2024
Closed
@s1gr1d
Copy link

s1gr1d commented Jul 22, 2024

@styfle can this be merged?

EndangeredMassa
EndangeredMassa suggested changes Aug 7, 2024
View reviewed changes
Copy link
Contributor

@EndangeredMassa EndangeredMassa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but there's one case we should add.

Thanks!

@AbhiPrasad AbhiPrasad mentioned this pull request Aug 8, 2024
Closed
styfle
styfle approved these changes Aug 29, 2024
View reviewed changes
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@EndangeredMassa Want to give it another look?

@s1gr1d s1gr1d mentioned this pull request Sep 2, 2024
Closed
@EndangeredMassa EndangeredMassa added the automerge Automatically merge PR once checks pass label Sep 3, 2024
@kodiakhq kodiakhq bot merged commit 37b3c16 into vercel:main Sep 3, 2024
@github-actions
Copy link

github-actions bot commented Sep 3, 2024

🎉 This PR is included in version 0.27.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@s1gr1d s1gr1d mentioned this pull request Sep 4, 2024
Closed
@leonardoadame leonardoadame mentioned this pull request Sep 24, 2024
Open
@Thompson1985 Thompson1985 mentioned this pull request Oct 7, 2024
Open
@styfle styfle mentioned this pull request Oct 26, 2024
Closed
@leonardoadame leonardoadame mentioned this pull request Nov 13, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@styfle styfle styfle approved these changes

@onsclom onsclom onsclom approved these changes

@ijjk ijjk Awaiting requested review from ijjk ijjk is a code owner

+1 more reviewer

@EndangeredMassa EndangeredMassa EndangeredMassa approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

automerge Automatically merge PR once checks pass released

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Does not include registered ESM loader hooks

5 participants

@timfish @styfle @s1gr1d @EndangeredMassa @onsclom