No longer able to exclude styles after upgrading from v12.0.4 to v12.0.5 about shepherd HOT 6 CLOSED

@RobbieTheWagner I'd always err on the side of caution and try to confirm things first, personally. 😅

So I created a totally fresh Vite project and took a look at what's going on. It's not all the styles being imported by default, it's just the styles for any Svelte component being added to the import graph (my demo only had the modal styles, for instance). That immediately excluded my prior guess that it was related to the Vite bug.

So I took a closer look at our earlier PR and I think this comes down to the Rollup config changes we made. From the Rollup Svelte Plugin README:

By default (when emitCss: true) the CSS styles will be emitted into a virtual file...
If you set emitCss: false and your Svelte components contain <style> tags, the compiler will add JavaScript that injects those styles into the page when the component is rendered. That's not the default, because it adds weight to your JavaScript, prevents styles from being fetched in parallel with your code, and can even cause CSP violations.

Our first approach involved creating a separate build for the CSS bundle and disabling that emitCss flag for the JS ones, but that's not quite correct given the behavior described above. Instead, we should re-enable emitCss with the postcss plugin for the ESM/CJS builds, delete the isolated CSS build, and then in package.json we can use exports to map to remap the CSS file:

    "./dist/css/*": {
      "import": "./dist/esm/css/*",
      "require": "./dist/cjs/css/*"
    },

Wildcard being used above, since that's just what I had tested with.

If we were so inclined, we could also throw in a copy operation to the ESM build to output an extra to ./dist/css/*, in case anyone is using old tooling that doesn't support the export remapping, though I don't think that's necessary?

I've tested the above in my Vite demo project and it seems to work!

I haven't had a chance to test it with a Webpack build though, nor have I run the E2E tests for this project against those changes. It's getting quite late, so that's something I'd have to tackle tomorrow.

Anyway, let me know if you'd like to pick this up. If you're busy, I'd be happy to look at this again tomorrow night and try to get the behavior confirmed and a PR submitted before I'm out of commission Thursday.

Thanks!

from shepherd.

@chuckcarpenter You got it! PR submitted.

from shepherd.

I wonder if this relates at all to Vite's #4389, where CSS files are treated as side-effectual by default and aren't tree shaken correctly. The Vue Shepherd project uses Webpack instead of Vite for bundling its E2E tests and I just tested it: not including an explicit import does not include the default CSS rules.

Might be worth spooling up a couple of skeleton projects for the two bundlers to clean room test it. If it's Vite-specific, it'll probably still need to be fixed (being one of the largest bundlers used), but at least that gives us an idea for why this suddenly happened.

Though I couldn't say why Vite wouldn't have been doing this before with the wildcard, it may be possible to leverage that as a fix by using a wildcard for this again (like /dist/css/*)?

from shepherd.

@Kenneth-Sills I think a wildcard might be a good fix here. Any way we can confirm easily or should we just try it?

from shepherd.

@Kenneth-Sills we really appreciate this work and would absolutely welcome a PR if you're able. It would be great to do another release ASAP to help folks who don't want these extra styles and restore previous workflows.

from shepherd.

Thanks for the quick response to this issue, great effort @Kenneth-Sills 👍🏽

from shepherd.