-
-
Notifications
You must be signed in to change notification settings - Fork 6.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: consider deep imports in isBuiltIn #5248
Conversation
would this also fix #4037? |
@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 |
Yes i think it will |
As far as I know, maybe we don't have directly tests for the utils, cause they are indirectly covered and tested 🤔 |
I'm curious though, should we use |
OK, I will try another way tomorrow, thx |
Some counterintuitive things blocked me. I tried to add a test case to the ssr project, and the module loaded via Use PR code first, reproduction: https://stackblitz.com/edit/vite-boyvb7?file=entry-server.js |
48e6887
to
86619aa
Compare
Well, let me explain the current situation🤔 I simply want to add a test case to ssr-vue, try to use To solve the error, I used The only way I can think of is to pass in Additionally, I added |
Oops, the error still exists. For some reasons, the use of vite/packages/vite/src/node/plugins/assetImportMetaUrl.ts Lines 33 to 38 in 9dbf20b
I try to avoid it, but it seems infeasible. @patak-js |
Ok, thanks for testing! |
fix(isBuiltIn): consider the case of deep import
I rebase to executable code. Thanks for your review 👍 |
@bluwy I think we should merge this one, could you also review it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Is this something you could show with a suggestion to @ygj6 ? |
Description
see detail: https://github.com/vitejs/vite/pull/5017/files#r725839810
...trying to add a test casedone
Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).