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 deep imports externalization (fixes #8420) #8421

Merged
merged 2 commits into from Jun 1, 2022

Conversation

patak-dev
Copy link
Member

Description

Fixes the error reported by @benmccann here #8420

But after this fix, SvelteKit is still not building as there is an issue with .css imports

kit.svelte.dev:build: TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".css" for /Users/patak/ecosystem/kit/node_modules/.pnpm/@sveltejs+site-kit@2.1.0/node_modules/@sveltejs/site-kit/base.css

We can still merge this one while we work on the other issue.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

benmccann
benmccann previously approved these changes May 31, 2022
@patak-dev patak-dev changed the title fix: respect package.exports when resolving SSR deep imports fix: SSR deep imports externalization (fixes #8420) May 31, 2022
@patak-dev
Copy link
Member Author

Tested SvelteKit and it is building after:
fix: don't externalize .css deep imports

We'll need to add later a .css dep in ssr test case in our CI

@patak-dev patak-dev added p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) feat: ssr labels May 31, 2022
@patak-dev patak-dev merged commit 89d6711 into main Jun 1, 2022
@patak-dev patak-dev deleted the fix/externalized-deep-import-extension branch June 1, 2022 05:18
// check ext before externalizing - only externalize
// extension-less imports and explicit .js imports
if (resolvedExt && !resolved.id.match(/(.js|.mjs|.cjs)$/)) {
return
Copy link
Member

Choose a reason for hiding this comment

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

Is this meant to be return resolved?

Copy link
Member Author

@patak-dev patak-dev Jul 17, 2022

Choose a reason for hiding this comment

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

No, it should return undefined. The only way this code is reached is when called to test if it should be externalized here. I should have added a comment there

true // try to externalize, will return undefined if not possible

Once this returns undefined, the second tryNodeResolve in the resolve plugin will already have externalize false so it wont reach the branch. This is quite confusing though. It was done as part of a refactoring that tried to not change too much of the current structure. Now that we know the new SSR externals logic is working, we should do another refactoring to clean up and avoid the need to calling tryNodeResolve twice (the ssr check is cached, so it is only duplicated the first time but still)

Copy link
Member

Choose a reason for hiding this comment

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

Oh wow that is very confusing, haha 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: ssr p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants