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

fix(ssr): no external symlink package #9296

Merged
merged 4 commits into from Jul 22, 2022
Merged

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Jul 22, 2022

Description

Fix sveltejs/kit#5542

If a package is symlinked, we should not externalize it by default as we usually treat them as source code instead, in this case, we no externalize it. This PR handles this.

Additional context

  • Also refactored the logic flow
  • The ssr external function only returns boolean without undefined as we're not using the undefined value anywhere

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.

undefined,
true,
true // try to externalize, will return undefined if not possible
false
Copy link
Member

@patak-dev patak-dev Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to pass true here or the branch testing deep imports will not be evaluated

Copy link
Member

@patak-dev patak-dev Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is also filtering .css files when passing true, we need more tests in ssr-deps 🤔

Copy link
Member Author

@bluwy bluwy Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I changed to false so we can resolve to the full path to analyze node_modules though. I'll try to find another way for this. Otherwise resolving linked-dep would return linked-dep, which doesn't give enough heuristic

Copy link
Member

@patak-dev patak-dev Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could do these checks inside tryNodeResolve when passing true

Copy link
Member Author

@bluwy bluwy Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the hint. Now the fix is a lot simpler than before 😅

Copy link
Member

@patak-dev patak-dev Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I think we still need a refactoring to make the branches more clear, we can get there in 3.1

@patak-dev patak-dev merged commit ea27701 into main Jul 22, 2022
13 checks passed
@patak-dev patak-dev deleted the fix-ssr-external-linked branch Jul 22, 2022
@brillout
Copy link
Contributor

brillout commented Jul 24, 2022

We should still respect ssr.external, right? E.g. vite-plugin-ssr should never be externalized, even if it's linked.

@brillout
Copy link
Contributor

brillout commented Jul 24, 2022

should never be externalized

never always

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: ssr p3-significant 🔨 High priority enhancement (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@sveltejs/kit and vite@3 in typescript monorepo fails with ERR_UNKNOWN_FILE_EXTENSION
3 participants