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

No longer able to exclude styles after upgrading from v12.0.4 to v12.0.5 #2847

Closed
bertiecroll opened this issue Jun 2, 2024 · 6 comments · Fixed by #2853
Closed

No longer able to exclude styles after upgrading from v12.0.4 to v12.0.5 #2847

bertiecroll opened this issue Jun 2, 2024 · 6 comments · Fixed by #2853
Assignees
Labels

Comments

@bertiecroll
Copy link

Hi all,

After upgrading from v12.0.4 to v12.0.5, the default shepherd css styles are now included even without the use of:
import 'shepherd.js/dist/css/shepherd.css';

Unfortunately for our usecase where we were not looking to use the shepherd css stylesheet and instead using our own css, the recent change has caused our tour styles to break due to the high specificity of the svelte component css rules.

expected behaviour
I can choose whether or not I want to use the default shepherd styles by including the shepherd.css stylesheet.

actual behaviour
All shepherdjs svelte component styles are added after using import Shepherd from 'shepherd.js'

shepherdjs svelte component styles included in head
Screenshot 2024-06-02 at 08 03 56

Related discussion on issue #2834.

@Kenneth-Sills
Copy link
Contributor

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/*)?

@RobbieTheWagner
Copy link
Member

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

@Kenneth-Sills
Copy link
Contributor

Kenneth-Sills commented Jun 4, 2024

@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!

@chuckcarpenter
Copy link
Member

@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.

@Kenneth-Sills
Copy link
Contributor

@chuckcarpenter You got it! PR submitted.

@bertiecroll
Copy link
Author

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

RobbieTheWagner added a commit that referenced this issue Jun 7, 2024
* fix: reconfigure CSS bundling

I've also added an E2E test group that gives us some basic monitoring
for regressions in the CSS inclusion behavior.

Closes #2847

* build: only distribute one copy of the css bundle

We still generate all three copies, we just delete the extra ones at the end
of the build process.

* Reduce the number of builds

---------

Co-authored-by: Robbie Wagner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants