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: support stylesheets with link tag and media/disable prop #6751

Merged
merged 5 commits into from
Jul 25, 2022

Conversation

pd4d10
Copy link
Contributor

@pd4d10 pd4d10 commented Feb 4, 2022

closes #6748

Description


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.

pd4d10 and others added 2 commits February 4, 2022 18:14
Co-authored-by: Thomas Steiner <tomac@google.com>
@patak-dev
Copy link
Member

Thanks for the PR @pd4d10 (and for reviewing @tomayac)

@pd4d10 would you review if you could add a test case so we avoid future regressions? There is info about extending the Vite test suite here https://github.com/vitejs/vite/blob/main/CONTRIBUTING.md#extending-the-test-suite. And you could add it to the css playground.

One issue with the fix is that the CSS sources will not be processed by the Vite pipeline. This means that when media and disabled are used instead of being ignored, a user could see their app break because post CSS is no longer working for them.

I don't know if we should risk that, as this feature was not requested for a long time. @pd4d10 would you check what would be involved to make this change while respecting the CSS pipeline. What I imagine is that we could still convert the CSS file to an import but add suffixes to the path like ?media=...&disabled. The CSS file would go through the pipeline, get transpiled, minified, but we would avoid bundling later CSS files that don't share the same media and disable attrs.

@pd4d10
Copy link
Contributor Author

pd4d10 commented Feb 4, 2022

The CSS file would go through the pipeline, get transpiled, minified, but we would avoid bundling later CSS files that don't share the same media and disable attrs.

Sounds reasonable. I'll look into it later

@Niputi Niputi changed the title fix: link tag with media prop fix: don't combine stylesheets with link tag and media/disable prop Feb 5, 2022
@Niputi Niputi added the p3-minor-bug An edge case that only affects very specific usage (priority) label Feb 5, 2022
@Niputi Niputi added this to the 2.9 milestone Feb 5, 2022
@patak-dev patak-dev modified the milestones: 2.9, 3.0 Mar 8, 2022
@patak-dev patak-dev removed this from the 3.0 milestone Jun 20, 2022
@bluwy
Copy link
Member

bluwy commented Jul 21, 2022

Added tests. This should be good now, and can confirm this fixes the issue.

@patak-dev
Copy link
Member

One issue with the fix is that the CSS sources will not be processed by the Vite pipeline. This means that when media and disabled are used instead of being ignored, a user could see their app break because post CSS is no longer working for them.

Given that apps using media and disabled are broken right now, I think we can merge this patch. I still think that we should make these scripts go through the pipeline later maybe as part of 3.1.
Thanks @pd4d10 and @bluwy!

@patak-dev patak-dev changed the title fix: don't combine stylesheets with link tag and media/disable prop fix: support stylesheets with link tag and media/disable prop Jul 25, 2022
@patak-dev patak-dev merged commit e6c8965 into vitejs:main Jul 25, 2022
@pd4d10 pd4d10 deleted the patch-6 branch July 25, 2022 16:37
@bluwy
Copy link
Member

bluwy commented Jul 26, 2022

Given that apps using media and disabled are broken right now, I think we can merge this patch. I still think that we should make these scripts go through the pipeline later maybe as part of 3.1.

Oh that seems like something we should definitely support, and I've completely missed your message before 😅 Will get to this when I have time soon.

@tomayac
Copy link
Contributor

tomayac commented Aug 2, 2022

Quick appreciation tweet. Thanks for fixing this! <3

smnscp added a commit to smnscp/stylament that referenced this pull request May 14, 2023
Due to vitejs/vite#6751 media-specific stylesheet links are ignored in the build pipeline and handled as static assets instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
No open projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

Vite lossily combines link[rel=stylesheet] and link[rel=stylesheet][media="(prefers-color-scheme:dark)"]
5 participants