-
-
Notifications
You must be signed in to change notification settings - Fork 1.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(vite-node): normalize relative paths to be from current working directory #952
Merged
Merged
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
e7d0db3
fix: Import with absolute paths
kalvenschraut e24e9f4
test: Added test case
kalvenschraut 6f10ede
fix: Move inside previous if
kalvenschraut d910208
fix: Cleanup test
kalvenschraut 6ff56c3
fix: Test typing
kalvenschraut 4e31d15
fix: Test wording
kalvenschraut 2c4ff7c
fix(resolveId): Update importer if it isn't from the root
kalvenschraut File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
export function dynamicRelativeImport(file: string) { | ||
return import(`./${file}.ts`) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
So after digging up I know why I didn't like this code from the first glance.
id
is treated as/src/path
instead of${root}/src/path
- this is whyresolveId
doesn't work. we should combineroot
andid
when needed and pass it tothis.options.resolveId
. or better teachresolveId
how to handle imports without root in them (resolveId
knowsroot
fromthis.server.options.root
)And
extname
has nothing to do with itSo, in the end it is
importer
that is wrong, not thedep
. This code inserver.ts
worked for me:Another path is to fix
resolveId
in Vite inselfThere 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.
ID looked like the path from the current directory to the file that has the import unless I miss understood something. I needed to check extname otherwise I was hitting problems with node internals and package names, but I guess shouldResolveId is handling those cases now?
Regardless I was just trying fix one of the few problems remaining so I could fully adopt this library for my companies code base, any suggested fix for my problem will be fine for me since you know the code base better. I like trying to provide a solution rather then making another issue. I can add the above to the server.ts code in my PR.
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.
Try to add this to
server.ts
in your PR, yeah. Thedep
value is fine - since it is not absolute, it will be resolved according toimporter
, butimporter
was incorrect. So the solution to your problem would be to fiximporter
.You also can import files without specifying the extension name, so the problem can rise again. I think the suggested solution is more well around.
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 updated my PR, I think I added it in the right spot. Test is still passing so seems to have worked.