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(glob): css imports injecting a ?used query to export the css string #6949

Merged
merged 9 commits into from Feb 20, 2022

Conversation

poyoho
Copy link
Member

@poyoho poyoho commented Feb 17, 2022

Description

fix: #6938

Additional context

import 'xx.css' had inconsistency.

in importAnalysisBuild plugins differentiate CSS imports, and add used query in the css import expression. glob / globEager feature also not default import but get emtry string.
this PR add the ?used in the glob / globEager import

Differentiate CSS imports that use the default export from those that
do not by injecting a ?used query - this allows us to avoid including
the CSS string when unnecessary (esbuild has trouble tree-shaking
them)


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.

packages/playground/css/main.js Outdated Show resolved Hide resolved
packages/vite/src/node/importGlob.ts Outdated Show resolved Hide resolved
@poyoho poyoho requested a review from bluwy February 20, 2022 08:55
bluwy
bluwy previously approved these changes Feb 20, 2022
@@ -94,12 +95,17 @@ export async function transformImportGlob(
)},`
} else if (isEager) {
Copy link
Member

Choose a reason for hiding this comment

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

I think the code is going to be more clear if we change it to

} else {
  const importeeUrl = isCSSRequest(importee) ? `${importee}?used` : importee
  if (isEager) {
    ...
  }
  else {
    ...
  }
}

We can avoid repeating the condition, and the main else makes it more evident that we are dealing with the non-raw case in both branches.

Copy link
Member Author

Choose a reason for hiding this comment

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

I seem to have found a pattern that when a fragment is reused it should be extracted. 😀

Copy link
Member

Choose a reason for hiding this comment

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

ha, not always, early abstraction is sometimes worse than duplicating code. But in this case, the variable applied to both branches when it was non-raw. I moved inside the else case, as it is not used in the raw branch.

@patak-dev patak-dev requested a review from bluwy February 20, 2022 13:30
@poyoho
Copy link
Member Author

poyoho commented Feb 20, 2022

thanks @patak-dev this is good change.

@poyoho
Copy link
Member Author

poyoho commented Feb 20, 2022

oh, It was already suggested, but I didn't notice it. Thanks for showing me the good case.

@patak-dev patak-dev merged commit 0b3f4ef into vitejs:main Feb 20, 2022
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@poyoho poyoho deleted the fix/css-glob-inconsistency branch February 20, 2022 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Glob imports do not work with CSS files
4 participants