Skip to content
This repository has been archived by the owner on Oct 13, 2022. It is now read-only.

CSS bundling no longer working as it used to be with Rollup 2 #233

Closed
habibrosyad opened this issue Jul 1, 2020 · 13 comments
Closed

CSS bundling no longer working as it used to be with Rollup 2 #233

habibrosyad opened this issue Jul 1, 2020 · 13 comments

Comments

@habibrosyad
Copy link

Hi, I'm using https://github.com/sveltejs/sapper-template-rollup as the base of my project months ago. Today, I updated packages as well as the rollup configs according to the latest commit of the template (particularly updating rollup from v1 to v2). However, I noticed that when building the app, all my css bundled into a single file only (main.css) whereas previously there were several css files (I guess it's based on the routes). Currently this behaviour breaks some of the styling in my project because there are conflicts between CSS rules.

Is there away to mitigate this issue (apart from rolling back prior to the update)?

An example for the list of css files generated with the vanilla dev build of the template (prior to update):

[slug].8afac42c.css
client.6bfaa544.css
index.7be4ff0d.css
index.9bb43f04.css
main.237455804.css

After update:

main.2065939480.css

The main reason that I noticed seems to be because of upgrading rollup v1 to v2. May need to adjust the rollup config, but I have no idea what to change.

@benmccann
Copy link
Member

I can confirm this. When visiting http://localhost:3000/profile/@ikzsl5 in the realworld sample it included http://localhost:3000/client/chunk.aef098f1.css until sveltejs/realworld#55

I have no idea whether this change was intended or how to affect the behavior. You'll probably have better luck asking questions about on Rollup on https://gitter.im/rollup/rollup or https://stackoverflow.com/questions/tagged/rollupjs or filing issues in https://github.com/rollup/rollup/issues. Please keep us updated in this issue though with any feedback or suggestions you receive from the Rollup community

@habibrosyad
Copy link
Author

Ok, will do.

@habibrosyad
Copy link
Author

@benmccann per the response in rollup/rollup#3655, they suggest that Sapper might not have been updated properly to support Rollup 2, therefore the breaking changes. Should this issue be brought up to Sapper instead?

@benmccann
Copy link
Member

Hmm. So I investigated more and I'm not quite sure why visiting http://localhost:3000/profile/@ikzsl5 in the realworld sample would include http://localhost:3000/client/chunk.aef098f1.css with Rollup 1 because the only style tag is in _error.svelte, so it actually seems maybe the Rollup 1 behavior was broken in some way too.

I think perhaps it will be better to investigate using the hn.svelte.dev sample

@habibrosyad
Copy link
Author

With the hn.svelte.dev sample and downgrading the Rollup to version 1 I could get different CSS being loaded when you visit different routes:

Visiting http://localhost:3000/top/1

<link rel="stylesheet" href="client/main.1788086292.css">
<link rel="stylesheet" href="client/[page].b543e849.css">
<link rel="stylesheet" href="client/client.cdab43ac.css">

Visiting http://localhost:3000/item/23703367

<link rel="stylesheet" href="client/main.1788086292.css">
<link rel="stylesheet" href="client/[id].465e0571.css">
<link rel="stylesheet" href="client/client.cdab43ac.css">

I suppose main and client seems to be the common CSS shared between those routes. Might as well unnecessary styles being loaded as you suggest there (weird behaviour with Rollup 1). However, with Rollup 2 everything just become a single file. Therefore, I think the later is much worse than the first one.

@benmccann
Copy link
Member

benmccann commented Jul 2, 2020

In Rollup 2, chunk.modules no longer contains the .css files and so the CSS files never get written to disk.

https://github.com/sveltejs/sapper/blob/be8db302fa58d4186b6ea0e03021e8cfe6f98348/src/core/create_compilers/extract_css.ts#L143

I'm not sure whether there's anything Sapper can do about this. That comes straight from Rollup. It seems most likely to need to be addressed in either Rollup or in rollup-plugin-svelte. rollup-plugin-svelte doesn't seem to be behaving any differently as far as I can see, but there may be some new requirement placed upon it which it's not fulfilling

https://github.com/sveltejs/sapper/blob/71d47b059f397cfb943bf470032efbd86ed94ec3/src/core/create_compilers/RollupResult.ts#L31

I pinned the Rollup version at 1.32.1 and it worked the old way and then pinned it to 2.0.0 and the behavior changed, so it was definitely related to the actual 2.0.0. Nothing in the release notes looks super obviously related - maybe the CSS files are now getting tree shaken or the return value of transform needs to change?

@benmccann
Copy link
Member

benmccann commented Jul 5, 2020

If I set treeshake: false then it works the way it did with Rollup 1. I'll leave a comment on the Rollup issue

@habibrosyad
Copy link
Author

habibrosyad commented Jul 7, 2020

Summing up based on the discussion in rollup/rollup#3655, another way to fix this without disabling treeshake all together would be using rollup/rollup#3663 (may need to wait for this to be released into master first) and update Sapper codes in https://github.com/sveltejs/sapper/blob/be8db302fa58d4186b6ea0e03021e8cfe6f98348/src/core/create_compilers/RollupCompiler.ts#L41-L46 into:

transform: (code: string, id: string) => {
  if (/\.css$/.test(id)) {
    this.css_files.push({ id, code });
    return {code: ``, moduleSideEffects: 'no-treeshake'};
  }
}

I have confirmed this using https://github.com/sveltejs/sapper/releases/tag/v0.27.16 (as well latest master) with the above changes and rollup/rollup#3663

@habibrosyad
Copy link
Author

habibrosyad commented Jul 7, 2020

@benmccann Should I make a PR for this in Sapper (while waiting for the update in Rollup to be released)? I've checked that those changes do not affect anything when running sapper project with the current template.

@benmccann
Copy link
Member

Cool. Let's wait until rollup/rollup#3663 is merged to confirm that will be the API, but then yeah it will make sense to make that change here

@benmccann
Copy link
Member

I also tested and confirmed the fix. I sent a PR here: sveltejs/sapper#1306

@habibrosyad
Copy link
Author

Thanks @benmccann, also rollup/rollup#3663 has been released in rollup@2.21.0, so the only things remains is for sveltejs/sapper#1306 to get merged.

@lukeed
Copy link
Member

lukeed commented Jul 29, 2020

Closed via sveltejs/sapper#1306 – thank you @benmccann

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants