-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
feat: add resolve.preserveSymlinks option #4708
Conversation
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.
Looks good and seems to have all the changes in the right places. The svelte (-kit) community thanks you 🥳
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.
Is it possible to add tests?
1095a0e
to
deca9fb
Compare
done |
@ygj6 we discussed this PR with the team and agreed that we should follow other bundlers/tools here, so this PR is approved to be merged. Since the default would be to now resolve the symlinks (same as in other tools), I think we should include this one in the beta for 2.6 so we give the ecosystem an opportunity to give us feedback. |
Co-authored-by: patak <matias.capeletto@gmail.com>
@@ -416,23 +453,28 @@ export function tryNodeResolve( | |||
basedir = root | |||
} | |||
|
|||
const preserveSymlinks = !!server?.config.resolve.preserveSymlinks | |||
|
|||
// nested node module, step-by-step resolve to the basedir of the nestedPath | |||
if (nestedRoot) { | |||
basedir = nestedResolveFrom(nestedRoot, basedir) |
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.
In #4634, I added a new nestedResolveFrom
function which uses resolveFrom
under-the-hood. We might want to update that function too:
vite/packages/vite/src/node/utils.ts
Line 71 in 284170e
export function nestedResolveFrom(id: string, basedir: string): string { |
with a preserveSymlinks
parameter as well. Don't think we need the ssr
parameter.
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.
Done, thank you for the reminder.
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.
Before merging see @bluwy https://github.com/vitejs/vite/pull/4708/files#r706991758 and at least give they an answer 🙂
Recently there was a problem with the mailbox, did not deal with this PR in time, thank you all review 😄. |
Thanks for implementing this feature @ygj6, we have now merged it in preparations for vite 2.6-beta |
Is |
Description
fix #4556, #2405.
Since I don't know much aboutpreserve-symlinks
, I've been working on this it for the past few days,I will add test cases and PR details later.refer:
esbuild#preserve-symlinks,
webpack#resolve.symlinks
node#preserve-symlinks
This commit(feat: add resolve.preserveSymlinks option) focuses on module dependencies,
vite/packages/playground/preserve-symlinks/package.json
Lines 10 to 12 in deca9fb
This commit(feat: properly resolve symbolic links that depend on imports) focuses on dependency imports
vite/packages/playground/preserve-symlinks/moduleA/src/index.js
Line 1 in deca9fb
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).