Skip to content
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

Merged
merged 6 commits into from Sep 13, 2021

Conversation

ygj6
Copy link
Member

@ygj6 ygj6 commented Aug 24, 2021

Description

fix #4556, #2405.

Since I don't know much about preserve-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,

"dependencies": {
"@symlinks/moduleA": "link:./moduleA"
}

This commit(feat: properly resolve symbolic links that depend on imports) focuses on dependency imports


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.

@Shinigami92 Shinigami92 added the p2-nice-to-have Not breaking anything but nice to have (priority) label Aug 24, 2021
dreitzner
dreitzner previously approved these changes Aug 25, 2021
Copy link

@dreitzner dreitzner left a 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 🥳

Copy link
Member

@Shinigami92 Shinigami92 left a 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?

@ygj6
Copy link
Member Author

ygj6 commented Aug 25, 2021

Is it possible to add tests?

done

@ygj6 ygj6 marked this pull request as ready for review August 25, 2021 14:19
Shinigami92
Shinigami92 previously approved these changes Aug 25, 2021
@patak-dev
Copy link
Member

@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.

@patak-dev patak-dev added this to the 2.6 milestone Aug 29, 2021
Co-authored-by: patak <matias.capeletto@gmail.com>
Shinigami92
Shinigami92 previously approved these changes Aug 30, 2021
antfu
antfu previously approved these changes Sep 10, 2021
@antfu antfu dismissed stale reviews from Shinigami92 and themself via 284170e September 13, 2021 03:55
@@ -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)
Copy link
Member

@bluwy bluwy Sep 13, 2021

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:

export function nestedResolveFrom(id: string, basedir: string): string {

with a preserveSymlinks parameter as well. Don't think we need the ssr parameter.

Copy link
Member Author

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.

antfu
antfu previously approved these changes Sep 13, 2021
Shinigami92
Shinigami92 previously approved these changes Sep 13, 2021
Copy link
Member

@Shinigami92 Shinigami92 left a 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 🙂

@ygj6 ygj6 dismissed stale reviews from Shinigami92 and antfu via 5b15546 September 13, 2021 08:07
@ygj6
Copy link
Member Author

ygj6 commented Sep 13, 2021

Recently there was a problem with the mailbox, did not deal with this PR in time, thank you all review 😄.

@patak-dev patak-dev merged commit b61b044 into vitejs:main Sep 13, 2021
@patak-dev
Copy link
Member

Thanks for implementing this feature @ygj6, we have now merged it in preparations for vite 2.6-beta

@zengxiaoluan
Copy link

zengxiaoluan commented Mar 21, 2024

Is preserveSymlinks possible to specify a particular file or directory to make effects?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Path includes a dot (.) character
7 participants