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.external/noExternal should apply to packageName #9146

Merged
merged 1 commit into from Jul 15, 2022

Conversation

patak-dev
Copy link
Member

@patak-dev patak-dev commented Jul 15, 2022

Description

Fixes

Also fixes https://github.com/Cluster2a/chartjsError, where we need now to add because of deps being external by default:

  ssr: {
    noExternal: ['chart.js']
  }

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@patak-dev patak-dev added the p4-important Violate documented behavior or significantly improves performance (priority) label Jul 15, 2022
@netlify
Copy link

netlify bot commented Jul 15, 2022

Deploy Preview for vite-docs-main canceled.

Name Link
🔨 Latest commit 259f91f
🔍 Latest deploy log https://app.netlify.com/sites/vite-docs-main/deploys/62d1d7e0fa40a2000aadfa55

@patak-dev
Copy link
Member Author

patak-dev commented Jul 15, 2022

cc @benmccann @dominikg @bluwy

We should check vite-ecosystem-ci and merge this PR, so it is included in Monday's release

@@ -120,13 +120,17 @@ export function createIsConfiguredAsSsrExternal(
return (id: string) => {
const { ssr } = config
if (ssr) {
if (ssr.external?.includes(id)) {
const pkgName = getNpmPackageName(id)
Copy link
Contributor

@dominikg dominikg Jul 15, 2022

Choose a reason for hiding this comment

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

getNpmPackageName returns '' for id='/some/path/with/leading/slash' which is falsy and returns undefined then. This is what we want, right?

Copy link
Member Author

@patak-dev patak-dev Jul 15, 2022

Choose a reason for hiding this comment

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

Only bare imports reach this check, so this is not possible 👍🏼

@patak-dev
Copy link
Member Author

patak-dev commented Jul 15, 2022

vite-ecosystem-ci run for this PR https://github.com/vitejs/vite-ecosystem-ci/actions/runs/2679454666, that is all green for prev working CIs

@patak-dev
Copy link
Member Author

patak-dev commented Jul 15, 2022

Merging as this is an important fix for downstream

@patak-dev patak-dev merged commit 5844d8e into main Jul 15, 2022
19 checks passed
@patak-dev patak-dev deleted the fix/ssr-external-internal-imports branch Jul 15, 2022
timacdonald added a commit to timacdonald/vite-plugin that referenced this pull request Jul 20, 2022
Due to a [bugfix](vitejs/vite#9146) in version
Vite 3.0.1 we now need to revert to referencing the package by name
directly in order to have Vite not externalise the helpers.

Fixes: laravel#93
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p4-important Violate documented behavior or significantly improves performance (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants