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): check root import extension for external #9494

Merged
merged 2 commits into from
Aug 9, 2022

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Aug 2, 2022

Description

Fix #9487
I don't think #8454 is the right fix, so I suppose this closes #8454 too.

We're previosly checking deep imports to no-external deps if they match a non-js file, we should also do this for root imports for normalize.css for example that exports css at the root.

Additional context

Slightly out-of-scope and more of a question: I'm not sure why below my PR changes we have special case handling for appending extension to resolved id. And why for deep imports only.


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.

@bluwy bluwy added feat: ssr p3-significant High priority enhancement (priority) labels Aug 2, 2022
@sapphi-red
Copy link
Member

Related: #8421

Slightly out-of-scope and more of a question: I'm not sure why below my PR changes we have special case handling for appending extension to resolved id. And why for deep imports only.

IIUC inside Vite handled files, we accept resolving pkg/foo to pkg/foo.js (when exports field does not exist), but in ESM this is not allowed. So we need to append extensions for the final bundle.
stackblitz

@bluwy
Copy link
Member Author

bluwy commented Aug 4, 2022

Ah thanks for the find! I think it's a bit odd still since we're externalizing the import so we shouldn't be modifying the import path 🤔 I'll try to figure out some cases locally.

@patak-dev patak-dev merged commit ff89df5 into main Aug 9, 2022
@patak-dev patak-dev deleted the fix-ssr-css-import branch August 9, 2022 09:15
@patak-dev
Copy link
Member

@bluwy it is as @sapphi-red describes, some of the React playgrounds were failing without this. The problem is that the import is in a file that is processed by Vite, it isn't external so if the user has .js in resolve.extensions, Vite will accept the deep import without extension. For reference, this is the original reason we migrated to using { external: true, ... } from hooks instead of directly defining an external function in Rollup.

@bluwy
Copy link
Member Author

bluwy commented Aug 9, 2022

Ah thanks! I didn't realize this is where we emulate all of Rollup's external behaviour, makes sense to handle those cases for ESM.

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.

Unknown file extension ".css"
3 participants