sveltejs / svelte

Cybernetically enhanced web apps
https://svelte.dev
MIT License
78.21k stars 4.09k forks source link

Svelte 5: per component css inject option #12656

Closed paoloricciuti closed 1 month ago

paoloricciuti commented 1 month ago

Describe the problem

As hinted in this PR #12374 it would be good to have the option to have injected css on a per component basis especially since the main usage of css injected is to render og images where you want the css to be injected only for that specific component.

Describe the proposed solution

It should be as simple as adding the option parsing in /src/compiler/phases/1-parse/read/options.js...i've actually already implemented it locally and i'm ready to push a PR for it but wanted to open the issue just to confirm that

  1. it's actually something needed and wanted (i was surprised it was this simple to implement since Rich left this detail out of that PR)
  2. my approach is actually correct and i'm not missing some important piece that will make this not work

From what i was able to see it seems everything is working...the css is generate only for the components that have this option and injected in the head both if the option-ated component is the only one or a nested one.

Importance

nice to have

dominikg commented 1 month ago

We recently discussed making compilerOptions in svelte.config.js optionally be a function similar to vite-plugin-svelte's dynamicCompileOptions with filename being an input to that function you could then pick per component.

Another thing (that could also be implemented on top) could be to also pass file content and somehow parse compile options (eg from frontmatter) before compiling it. That could be overkill, but you could eg use it to see if the custom element option is set and compile with that flag. Or hide warnings for node_modules files or ...

paoloricciuti commented 1 month ago

We recently discussed making compilerOptions in svelte.config.js optionally be a function similar to vite-plugin-svelte's dynamicCompileOptions with filename being an input to that function you could then pick per component.

Another thing (that could also be implemented on top) could be to also pass file content and somehow parse compile options (eg from frontmatter) before compiling it. That could be overkill, but you could eg use it to see if the custom element option is set and compile with that flag. Or hide warnings for node_modules files or ...

Yeah in this moment i'm using dynamicCompileOption for this but have the option in the component itself is nicer imho.

dummdidumm commented 1 month ago

All sounds good, go ahead ๐Ÿ‘

paoloricciuti commented 1 month ago

All sounds good, go ahead ๐Ÿ‘

Great, opened it ๐Ÿ˜„

dominikg commented 1 month ago

uhh, do we really want 2 ways to do this? if we do, I'm going to need the compiler returning the final options used so vps knows if css was injected or not. otherwise it can't handle the virtual css module correctly.

dominikg commented 1 month ago

This also means that if a svelte library added it to its published components, it would prevent the user of that library from externalizing the css.

paoloricciuti commented 1 month ago

This also means that if a svelte library added it to its published components, it would prevent the user of that library from externalizing the css.

The component that doesn't have the option is still externalizing the css tho.

going to need the compiler returning the final options used

So from the render function?

dominikg commented 1 month ago

The component that doesn't have the option is still externalizing the css tho.

doesn't matter if its one or many or all. Today, the code calling .compile is in control over the compilerOptions. This changes it so that the author of the code to be compiled can override it.

So from the render function?

no, the return value of svelte.compile

paoloricciuti commented 1 month ago

doesn't matter if its one or many or all. Today, the code calling .compile is in control over the compilerOptions. This changes it so that the author of the code to be compiled can override it.

So if one component uses this the whole application will inject css?

dominikg commented 1 month ago

no, but if the application is compiled with external and emitCss, vite-plugin-svelte is going to try and emit that css, oblivious to the fact that this one component was compiled with injected even though it passed in external.

And it doesn't matter if that happens to one or all components because it is bad.

Edit: this sets a precedent, giving component authors authority over how that code is going to be compiled which is a paradigm shift because today it is the application setup with svelte.config.js that has exclusive reign over it.

Edit2: it'll also make that code less portable if we ever change how that option works because it is hardcoded published to npm.

What exactly is the advantage here? That you can publish a library that generates og images? A small bit of documentation how to ensure its compiled correctly via new dynamic compile options feature would help with that. The library could even export code that helps with it.

paoloricciuti commented 1 month ago

no, but if the application is compiled with external and emitCss, vite-plugin-svelte is going to try and emit that css, oblivious to the fact that this one component was compiled with injected even though it passed in external.

And it doesn't matter if that happens to one or all components because it is bad.

Edit: this sets a precedent, giving component authors authority over how that code is going to be compiled which is a paradigm shift because today it is the application setup with svelte.config.js that has exclusive reign over it.

Edit2: it'll also make that code less portable if we ever change how that option works because it is hardcoded published to npm.

What exactly is the advantage here? That you can publish a library that generates og images? A small bit of documentation how to ensure its compiled correctly via new dynamic compile options feature would help with that. The library could even export code that helps with it.

Tbf this was just a nice to have so that you don't have to setup dynamicCompileOption but if it creates more problem that it solves we can just avoid doing this. In the PR that introduced injected css Rich mentioned this options possibility and when I had to use it in sveltelab i actually whished o had this option but it's perfectly fine to avoid it.

Conduitry commented 1 month ago

I agree that building something like v-p-s's dynamic compiler options into the compiler itself (in a general way) sounds like a better idea than adding this as a <svelte:options> option.

dominikg commented 1 month ago

I mean there can be value in library authors telling users what compile options they expect and svelte5 got a bit stricter with bindable too, this could be seen like a similar restriction.

But we have to be careful to add it in a way that does not prevent future changes to the compiler options api or introduces breaking changes to svelte as a language accidentally.

Rich-Harris commented 1 month ago

giving component authors authority over how that code is going to be compiled which is a paradigm shift because today it is the application setup with svelte.config.js that has exclusive reign over it

To be clear, this isn't true โ€” there are currently four options that can be set inline, two of which are deprecated:

If someone could override the component author's wishes on any of these, it would just break stuff.

paoloricciuti commented 1 month ago

I would say that since this is just a nice to have and it would require huge work in v-p-s and the svelte:options route also seems dangerous i think we can close this and the PR and maybe try to think of something to make this work dynamically. I wonder if this could be an option on the render function instead. That would fix the problem of having to change your svelte config just to render an og image without causing problems to v-p-s right?

Rich-Harris commented 1 month ago

I wonder if this could be an option on the render function instead

If it was that easy we'd have done that in the first place :)

It needs to be a compiler option. We don't want to generate the CSS inline for every component just in case someone uses that option, and we can't just expose it as a treeshakeable export because there's no way to account for nested components. The question is whether it's a compiler option that is colocated with the code it concerns, or something that has be set at a difference in a way that's very liable to break unexpectedly, generate waste from false positives, and be rather hard to debug.