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

scoped style doesn't work if compile component with css option 'none' #8465

Closed
dhcmrlchtdj opened this issue Apr 8, 2023 · 15 comments
Closed
Labels
awaiting submitter needs a reproduction, or clarification
Milestone

Comments

@dhcmrlchtdj
Copy link

Describe the bug

As #7914 said:

When hydrating SSR components, the styles are commonly already generated and there is no need to generate it again for the client side bundle.

However, {css:"none"} does not work well with scoped styles.

Reproduction

Currently, the REPL doesn't support none (and I created a PR to support it sveltejs/sites#460).

Let's just consider the following code snippet:

<div> red </div>
<style> div { color: red } </style>

We can compile this code using {generate: "dom", css: "external"} to obtain two output that are used for CSR:

/* output.css */
div.svelte-d8zsiy{color:red }
/* output.csr.html */
<div class="svelte-d8zsiy">red</div>

And then we re-compile this code using {generate: "ssr", css: "none"}, there will be no CSS or scoped class name generated.
As a result, the output.css file will not be applied to the SSR HTML:

# output.ssr.html
<div>red </div>

With {css: "none"}, the style is completely ignored, which means that Svelte cannot calculate the hash for the scoped class name.


A simple solution to this problem is to use {generate: "ssr", css: "external"} to generate the HTML for SSR.
But, as you can see in the REPL, all CSS are contained in the JS output, resulting in a larger bundle size.
Although bundle size for SSR might not be a problem in many cases, it could be beneficial to remove these unused CSS in JS output. (TBH, in my case, I run svelte inside the service worker, which means bundle size is critical.)

I propose that we reopen #2360 as it provides a simple and effective solution to remove CSS from the JS output.

Logs

No response

System Info

svelte 3.58.0

Severity

annoyance

@dummdidumm
Copy link
Member

Rich's comment in that closed PR still is correct - why not just ignore the output? I also don't understand how this is related to bundle size in a web worker. Setting the option to none won't make the compiler smaller, and if you discard the css output the result won't be bigger.

@dummdidumm dummdidumm added the awaiting submitter needs a reproduction, or clarification label Apr 8, 2023
@dhcmrlchtdj
Copy link
Author

I created a repository to demonstrate the issue since it's difficult to explain without code.
https://github.com/dhcmrlchtdj/svelte-8465/tree/main/build

My workflow look like

  • at build time
    • compile with { generate: "dom", css: "external" }, which generates dom_external/output.{js,css}
    • dom_external/output.{js,css} are added to app shell
    • compile with { generate: "ssr", css: "external" }, which generates ssr_external/output.{js,css}
    • ssr_external/output.js is added to service worker
    • ssr_external/output.css is discarded
  • at render time
    • service worker will invoke ssr_external/output.js to generate ssr_external/step2.{html,css}
    • ssr_external/step2.html will be injected into app shell
    • ssr_external/step2.css will be discarded

Since ssr_external/output.js is shipped to browser, I want to minimize the it's size as much as possible.
As can be seen in the ssr_external/output.js, all CSS is inclueded. But the generated ssr_external/step2.css is never used.


Now, let's talk about {css: "none"}. To reduce the size of output.js, I tried to compile with none.

  • at build time
    • { generate: "ssr", css: "none" }, which generates ssr_none/output.{js,css}
    • ssr_none/output.js is much smaller than ssr_external/output.js, that is what I want
    • ssr_none/output.css is empty
  • at render time
    • ssr_none/output.js will generate ssr_none/step2.{html,css}
    • ssr_none/step2.css is empty, this is expected
    • but ssr_none/step2.html doesn't work with dom_external/output.css, this is what the bug I want to report

Compare ssr_none/step2.html with ssr_external/step2.html, the scoped class name is missed.


Returning to my proposal, it is just an optimization for my particular scenario.
Usually, ssr_external/output.js would not be sent to the browser, so the bundle size is not a major concern.
However, given that we have already specified css: "external", there is no reason to retain the CSS within ssr_external/output.js.
That could benefic for edge cases like mine.

@andy0130tw
Copy link
Contributor

Normally, the generated CSS requires post-processing by a build system to rewrite all links to assets. However, when building with Vite with ssr: true, setting css: "external" in Svelte's compiler option will not output the CSS. The embedded CSS, although a static string for each component, is only available after a completed call to render(...).

I doubt the current usefulness of CSS in the SSR build result. I notice that SvelteKit does not use the content in $$result.css at all, and instead serves the CSR'ed CSS. Sure, we can ignore it, but maybe this design has its own purpose that I haven't realized?

@dhcmrlchtdj
Copy link
Author

The svelte playground has been upgraded, enabling us to illustrate the problem more conveniently now.
Let's use https://svelte.dev/examples/styling as an example.

This is the matrix illustrating how the option affects the JS output.

dom ssr
injected generate css (append_styles) and class (attr) generate css and class
external generate class (attr) generate css and class
none no css or class generated no css or class generated

As you can see, css: injected/external does not affect the JS output.

My proposal is to remove css from the JS output when we use {generate:"ssr",css:"external"}, similar to what we did with {generate:"dom",css:"external"}.

The goal is very similar to what we did with #7914, which introduced css:none to remove both css and class.
However, I want to keep the class to avoid breaking the scoped CSS.

From my perspective, I believe that this change is not a significant breaking change.
People who want to generate both css and class could use {generate:"ssr",css:"injected"} to keep the original output.

cc: @dummdidumm

@dummdidumm
Copy link
Member

dummdidumm commented Jun 26, 2023

@bluwy / @dominikg / @Conduitry is anyone of you aware if any tooling uses the CSS code and map the SSR renderer appends to the $$result object? Should the logic here follow the same logic as the dom renderer and if it's css: "external" then omit that CSS string, or is this semantically different here?

Example of what I'm talking about:

// SSR compilation output:
/* App.svelte generated by Svelte v4.0.0 */
import { create_ssr_component } from "svelte/internal";

// do we need this css object?
const css = {
	code: "p.svelte-urs9w7{color:purple;font-family:'Comic Sans MS', cursive;font-size:2em}",
	map: "{\"version\":3,\"file\":\"App.svelte\",\"sources\":[\"App.svelte\"],\"sourcesContent\":[\"<p>Styled!</p>\\n\\n<style>\\n\\tp {\\n\\t\\tcolor: purple;\\n\\t\\tfont-family: 'Comic Sans MS', cursive;\\n\\t\\tfont-size: 2em;\\n\\t}\\n</style>\\n\"],\"names\":[],\"mappings\":\"AAGC,eAAE,CACD,KAAK,CAAE,MAAM,CACb,WAAW,CAAE,eAAe,CAAC,CAAC,OAAO,CACrC,SAAS,CAAE,GACZ\"}"
};

const App = create_ssr_component(($$result, $$props, $$bindings, slots) => {
	$$result.css.add(css); // can we omit this (and therefore the above object) if css: 'external' ? Who uses this?
	return `<p class="svelte-urs9w7">Styled!</p>`;
});

export default App;

@bluwy
Copy link
Member

bluwy commented Jun 26, 2023

I don't think any tooling is using that (the map). I remember wanting to remove that in Svelte 4 and totally forgot 😅 The only breaking change would be if some tool trying to statically extract for some reason. Dug up some extra context:

Re $$result.css, I don't quite remember, but I believe that's not used too. it seems to be used at

css: {
code: Array.from(result.css)
.map((css) => css.code)
.join('\n'),
map: null // TODO
},

@dummdidumm
Copy link
Member

I'm tempted to make the change that when css is set to external that the css is not filled in 4.0.1 and mark it as bugfix with the extra leeway that we just did breaking changes so one that _could theoretically _ be seen as one is ok

@bluwy
Copy link
Member

bluwy commented Jun 27, 2023

I remember having that same thought for Vite 4.0.1, made the change, and made a few people unhappy 😄 But since this is more internal stuff, and injected & external are (strangely) doing the same thing, I'd be fine by that.

@dominikg
Copy link
Member

I prefer a strict view on semver. Unless this is completely broken, has no usecase and changing it fixes a bug, it should wait for 5.0.

Also if you move ahead, check with ecosystem-ci and users who possibly enabled devSourcemaps in Vite. I remember at least one bugfix we made in vps to accomodate

@dummdidumm
Copy link
Member

So how about this: If SSR source maps are disabled AND the css is set to external then we can be sure that the returned CSS object is of no use so we can omit it.

@dhcmrlchtdj
Copy link
Author

@dummdidumm, thank you for your efforts to address this issue.

Currently, both {generate,css} affect JS output.
The problem we face is that the output of {generate:"ssr",css:"external"} is more complex than expected.

Introducing the {enableSourcemap} into the matrix would further increase the complexity.
From my perspective, this approach is not preferable.

I'm confused by the sourcemap concern, how could it be related to the css:external/injected. Since we are working on the js.code, it should be orthogonal to the sourcemap.
@andy0130tw also mentioned that SvelteKit does not use the content in $$result.css at all.

┌────────────────────────┐
│svelte.compile(src,opts)│
└───────────┬────────────┘
            ▼
┌─────────┬──────────────┐
│         │   code       │
│  js     ├──────────────┤
│         │   map        │
├─────────┼──────────────┤
│         │   code       │
│  css    ├──────────────┤
│         │   map        │
└─────────┴──────────────┘

I'm attempting to run tests with a patched svelte on svelte-kit and vite-svelte-plugin.
Setting up the env has been quite challenging...

@dominikg
Copy link
Member

There are sourcemaps for CSS too, not just JS. If users enable these, they should work. Not emitting them in that case would break their usecase

@dummdidumm
Copy link
Member

But shouldn't these people use the css: { code: string; map: string } object returned by the Svelte compiler for this just like they do for the dom case? It makes zero sense for me that SSR also adds these. @Conduitry do you have any historical insight here why this was done?

@dhcmrlchtdj
Copy link
Author

I found this topic has been discussed before, #3604.

Rich Harris said that it's not designed to have any effect in SSR mode.

But I didn't find any strong objection to removing it from the generated js.code.

@dummdidumm dummdidumm added this to the one day milestone Jul 5, 2023
@Rich-Harris
Copy link
Member

Closing since CSS is no longer included in JS output in Svelte 5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting submitter needs a reproduction, or clarification
Projects
None yet
Development

No branches or pull requests

6 participants