Skip to content

fix(glob): css imports injecting a ?used query to export the css string#6949

Merged
patak-cat merged 9 commits intovitejs:mainfrom
poyoho:fix/css-glob-inconsistency
Feb 20, 2022
Merged

fix(glob): css imports injecting a ?used query to export the css string#6949
patak-cat merged 9 commits intovitejs:mainfrom
poyoho:fix/css-glob-inconsistency

Conversation

@poyoho
Copy link
Copy Markdown
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.

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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-cat patak-cat requested a review from bluwy February 20, 2022 13:30
@poyoho
Copy link
Copy Markdown
Member Author

poyoho commented Feb 20, 2022

thanks @patak-dev this is good change.

@poyoho
Copy link
Copy Markdown
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-cat patak-cat merged commit 0b3f4ef into vitejs:main Feb 20, 2022
Copy link
Copy Markdown
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 👍

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