fix: resolve addons using nodeResolve()

[ygj6]

Description

fix: #5709

Passed the test using the bcrypt module, but did not test the @prisma/client module mentioned in #5709 because I don't have a clue about the example of using this module. Can you provide further help like another replication repo? @cyco130

Additional context

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the Commit Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[patak-dev]

@ygj6 nice! Do you know why this was a regression? I don't see many places in the code where we are checking for .node, only see one in resolveFrom in utils.

About the test case, I'm thinking that we may want to create a new vanilla playground/ssr-deps where we can have all of these cases. And where possible replace real dependencies with local minimal reproductions.

[ygj6]

@patak-js I think this is because .node is a C++ addons for nodejs, docs here. We use require() to load .node files like built-in modules.

About the test case, I'm thinking that we may want to create a new vanilla playground/ssr-deps where we can have all of these cases. And where possible replace real dependencies with local minimal reproductions.

Good idea! I can issue a new PR for this test case.

[cyco130]

Passed the test using the bcrypt module, but did not test the @prisma/client module mentioned in #5709 because I don't have a clue about the example of using this module. Can you provide further help like another replication repo? @cyco130

Prisme generates the @prisma/client module, so its dependencies are determined by your Prisma schema. I'm positive that what didn't work was a native Postgres dependency. It's not really possible to create a minimal real repro without having to set up a whole database. But I can offer to test as soon as the new beta is available.

[cyco130]

On further inspection, Prisma problem is also related to Cannot find module '_http_common' which is a builtin node module (like fs etc.). Maybe the internal module list is not up to date? Should I open a new issue?

[cyco130]

Here's a repro: https://github.com/cyco130/vite-ssr-http-common-issue

node ssr-module.mjs succeeds but node . fails trying to load the same module with ssrLoadModule. Let me know if this doesn't fix it so I can open a new issue.

[ygj6]

@cyco130 I tested it locally with the latest code, it is a separate issue, thanks for your help!

[ygj6]

I'm sure that the problem lies here.

https://github.com/vitejs/vite/blob/main/packages/vite/src/node/plugins/importAnalysis.ts#L370

This condition is not working properly because filtering is performed in the source code of builtin-modules that isBuiltin depends on, which excludes the _http_common module.

https://github.com/sindresorhus/builtin-modules/blob/main/index.js#L10

I don’t know the reason for this, can you take a look? @patak-js

[cyco130]

Opened an issue with an explanation of the possible cause: #5826

[patak-dev]

I checked builtin-modules history and the filtering for ^_ modules was there from the beginning. We already discussed this lib here #5248 (comment), @bluwy you wanted to replace it, maybe we have a reason now? Do you know why ^_ like _http_common aren't considered builtins?

[patak-dev]

There was a similar issue against esbuild evanw/esbuild#1294

esbuild is maintaining a harcoded list, should we do the same?
https://github.com/evanw/esbuild/blob/824d41a9856c82b377251cb2bddfd856b6e57d65/internal/resolver/resolver.go#L1892

// This list can be obtained with the following command:
//
//   node --experimental-wasi-unstable-preview1 -p "[...require('module').builtinModules].join('\n')"
//
// Be sure to use the *LATEST* version of node when updating this list!

[bluwy]

you wanted to replace it, maybe we have a reason now?

I wanted to replace it by calling from node's provided builtinModules, but I believe from that comment we decided not to since we want to detect node16 modules in the context of node12 too (for example).

Do you know why ^_ like _http_common aren't considered builtins?

I'm not sure why, it could be an unintentional safeguard.

esbuild is maintaining a harcoded list, should we do the same?

I think that sounds like a good plan 👍

[patak-dev]

@ygj6 what do you think? We are using esbuild anyways, so we will have issues because of their list if we don't do the same. The ideal would be that this is shared somehow, but I think we could hardcode now and check hot to make this more robust later.

[ygj6]

@patak-js

we could hardcode now and check hot to make this more robust later.

I agree with that, this seems to be a more feasible way at the moment.

The ideal would be that this is shared somehow

Anything can promote this?

[patak-dev]

The ideal would be that this is shared somehow

Anything can promote this?

I don't think that Evan Wallace would like to expose an API for us to get this info, looks that it is out of scope for esbuild. Maybe the best would be to fix builtin-modules which is used by tons of projects?

[ygj6]

Maybe the best would be to fix builtin-modules which is used by tons of projects?

I think @bluwy has done some investigations in our previous discussion #5248 (comment), right? builtin-modules still has its issue waiting to be resolved, in that case, let's solve it by ourselves, since there is already a PR there

[bluwy]

Yeah, builtin-modules doesn't list nested modules yet like fs/promises (sindresorhus/builtin-modules#12). We could probably do that ourselves at the meantime.