fix: consider deep imports in isBuiltIn

[ygj6]

Description

see detail: https://github.com/vitejs/vite/pull/5017/files#r725839810

...trying to add a test case
done

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.

[Niputi]

would this also fix #4037?

[ygj6]

@patak-js @Shinigami92 I don’t see e2e tests for other utils functions, so I’m a bit confused whether I need to add test cases

[ygj6]

would this also fix #4037?

Yes i think it will

[Shinigami92]

@patak-js @Shinigami92 I don’t see e2e tests for other utils functions, so I’m a bit confused whether I need to add test cases

As far as I know, maybe we don't have directly tests for the utils, cause they are indirectly covered and tested 🤔

[bluwy]

I'm curious though, should we use import { builtinModules } from 'module' instead of the builtin-modules package?

[ygj6]

@patak-js @Shinigami92 I don’t see e2e tests for other utils functions, so I’m a bit confused whether I need to add test cases

As far as I know, maybe we don't have directly tests for the utils, cause they are indirectly covered and tested 🤔

OK, I will try another way tomorrow, thx

[ygj6]

Some counterintuitive things blocked me. I tried to add a test case to the ssr project, and the module loaded via ssrLoadModule raised the error ReferenceError: __dirname is not defined. Not sure what I am missing, can anyone provide an explanation or help?

Use PR code first, reproduction: https://stackblitz.com/edit/vite-boyvb7?file=entry-server.js

[ygj6]

Well, let me explain the current situation🤔

I simply want to add a test case to ssr-vue, try to use readFile(path.resolve(__dirname,'./src/message'),'utf-8') in entry-server.js to read the file, and then encountered the error ReferenceError: __dirname is not defined.

To solve the error, I used readFile(new URL('./src/message', import.meta.url)) to read the file according to the nodejs issue and its docs, but this caused the test to fail.

The only way I can think of is to pass in __dirname to entry-server.js. This works when node version >=14 but fails on version 12 because 'fs/promises' API is available since version 14.

Additionally, I added process.versions.node to conditionally get fs to make the test pass. I’m not sure if this is appropriate, please review.

[patak-dev]

@ygj6 could the problem you encountered with import.meta.url be solved now that we merged #5268? Maybe you could rebase to main and see if the workaround is still needed

[ygj6]

Oops, the error still exists. For some reasons, the use of new URL is not allowed in the SSR phase.

vite/packages/vite/src/node/plugins/assetImportMetaUrl.ts

Lines 33 to 38 in 9dbf20b

	if (ssr) {
	this.error(
	`\`new URL(url, import.meta.url)\` is not supported in SSR.`,
	index
	)
	}

I try to avoid it, but it seems infeasible. @patak-js

[patak-dev]

Ok, thanks for testing!

[ygj6]

I rebase to executable code. Thanks for your review 👍

[patak-dev]

@bluwy I think we should merge this one, could you also review it?

[bluwy]

I still think using builtinModules from module is the more robust way (if there isn't any gotchas), but otherwise this technique looks fine too.

[patak-dev]

Is this something you could show with a suggestion to @ygj6 ?