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

feat: support importing css with ?raw #5796

Merged
merged 4 commits into from Mar 25, 2022

Conversation

stygian-desolator
Copy link
Contributor

Description

close #5724


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.

@Shinigami92 Shinigami92 marked this pull request as draft November 22, 2021 14:36
@Shinigami92 Shinigami92 self-requested a review November 22, 2021 14:36
@Shinigami92
Copy link
Member

support importing css with ?raw is this a fix or a feat? 🤔

@Niputi
Copy link
Contributor

Niputi commented Nov 22, 2021

looks like a new feature

@Shinigami92
Copy link
Member

looks like a new feature

In this case we should add p2, rename PR and potentially add to next meeting notes to discuss if there is not something overseen. I darkly remember that something was different with css?raw any why it was not there yet 🤔 But I don't know anymore 😕

@Niputi Niputi added the p2-nice-to-have Not breaking anything but nice to have (priority) label Nov 22, 2021
@gluck
Copy link
Contributor

gluck commented Nov 23, 2021

The PR title does sound like a feat, but I'll try to argue given that:

  • raw css import is already supported from local files (though it may be unintended), it only fails when loading a css file from node_modules/ (was my initial thought but not exactly)

  • raw css import is already supported when CSS file don't require CSS transform (though it may be unintended), it fails when they do (scss file, contains url(), is css module ...)

  • doc mentions ?raw to load any asset, why not css ?

  • current behavior fails because vite:css transform tries to parse raw code (JS) as css code, that hardly sounds "on-purpose" either

My 2c (disclaimer: I'm the one who reported and needs the fix, so I'm probably biaised ! please pursue as you see fit, and thanks for the PR !)

@stygian-desolator stygian-desolator force-pushed the fix/css-raw-import branch 4 times, most recently from 3ce9c7f to 545ba11 Compare November 23, 2021 15:53
@Shinigami92
Copy link
Member

doc mentions ?raw to load any asset, why not css ?

You might have misunderstood this. I thing css doesn't count as asset but known usable file. Where as glsl or something like that is a real asset. Like in the example in the docs.

@gluck
Copy link
Contributor

gluck commented Nov 23, 2021

doc mentions ?raw to load any asset, why not css ?

You might have misunderstood this. I thing css doesn't count as asset but known usable file. Where as glsl or something like that is a real asset. Like in the example in the docs.

Fair enough, though the line may be thin, ?raw can be used today (and works fine, though maybe unintended again) on JSON & JS files FWIW.

@stygian-desolator
Copy link
Contributor Author

It's strange, some old test cases keep failing with the error message

elementHandle.evaluate: Execution context was destroyed, most likely because of a navigation.

and I also see the phrase at the top of the css.spec.ts file

note: tests should retrieve the element at the beginning of test and reuse it in later assertions to ensure CSS HMR doesn't reload the page

am I missing something?

@stygian-desolator stygian-desolator force-pushed the fix/css-raw-import branch 2 times, most recently from f5af289 to 66f1efd Compare November 24, 2021 15:23
@stygian-desolator stygian-desolator marked this pull request as ready for review November 25, 2021 01:09
Shinigami92
Shinigami92 previously approved these changes Nov 25, 2021
@jackmcpickle
Copy link

jackmcpickle commented Dec 20, 2021

Also hitting this issue, Use case for us it to import raw css file to be using in pagejs for print styles.

IMO if I declare something to be raw - like the raw-loader in webpack, I should get it raw.

Not sure if this could be accomplished with a postcss plugin maybe?

bluwy
bluwy previously approved these changes Dec 20, 2021
Shinigami92
Shinigami92 previously approved these changes Dec 26, 2021
@Shinigami92
Copy link
Member

Please rename title before merging to feat: or to reflect the fix

@stygian-desolator stygian-desolator changed the title fix: support importing css with ?raw feat: support importing css with ?raw Dec 26, 2021
bluwy
bluwy previously approved these changes Dec 28, 2021
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.

I made a comment at the other PR:

Evan's comment here might be relevant as well, and I'm not sure if there's more needed to address that.

I think it's fine for now as it'll make ?url less broken until we officially support css ?url (if there happens to be a bug within it)

@matthewp
Copy link
Contributor

@bluwy yep, can close that one.

@mvolfik
Copy link
Contributor

mvolfik commented Feb 14, 2022

I just ran into this issue and before I realized this PR exists, I implemented a fix in a very similar way :))

Therefore sorry for nagging, but is there anything this PR is waiting for? It would really help me if this was merged...

bluwy
bluwy previously approved these changes Feb 17, 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. Though I still have reserves for Evan's comment on ?url #2522 (comment)

@gluck
Copy link
Contributor

gluck commented Feb 18, 2022

LGTM. Though I still have reserves for Evan's comment on ?url #2522 (comment)

I think that comment applied only assets which require transpilation (e.g. scss), for which a user could expect file.scss?url|raw to return a useable/compiled css asset (but that sounds both a niche and a complex use case), and not a link to the scss raw file because that would likely be unuseable at runtime.

This change implements the later, and while I agree it probably makes no sense for scss files, it's still both:

  • perfectly valid for css files, which can be used at runtime as-is
  • way better than the current behavior (which tries to parse raw/url javascript output as css, which is guaranteed to fail with a cryptic parsing error)

@mvolfik
Copy link
Contributor

mvolfik commented Feb 18, 2022

Sorry for dispersing the discussion into a PR, but my .02$ on this: I never needed the scss?url use-case, but I can imagine needing the compiled css for link embedding. On the other hand, I'd absolutely always expect the ?raw import to give the raw, source file, not compiled css.

However, this sounds a bit inconsistent, so it might be worth introducing some distinction like ?url&compiled, ?url, ?raw&compiled etc.

Also, it would be great if all those prefixes were briefly documented in one place, alongside the ?worker etc.

Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

We discussed the PR today with the team, we are good to merge it 👍🏼

@mule-stand
Copy link

Sorry for dispersing the discussion into a PR, but my .02$ on this: I never needed the scss?url use-case, but I can imagine needing the compiled css for link embedding. On the other hand, I'd absolutely always expect the ?raw import to give the raw, source file, not compiled css.

However, this sounds a bit inconsistent, so it might be worth introducing some distinction like ?url&compiled, ?url, ?raw&compiled etc.

Also, it would be great if all those prefixes were briefly documented in one place, alongside the ?worker etc.

did you achieve this somehow? I also faced with the fact that I want to embed compiled css tailwind in iframe to isolate a part of page

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: css p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Raw css import from node_modules fails
10 participants