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

single stories are pulling in every other stories' stylesheets? #729

Closed
moimikey opened this issue Mar 21, 2017 · 72 comments
Closed

single stories are pulling in every other stories' stylesheets? #729

moimikey opened this issue Mar 21, 2017 · 72 comments

Comments

@moimikey
Copy link
Contributor

moimikey commented Mar 21, 2017

Hi,

I'm trying to figure out why i'm having this particular issue. I'm dynamically loading stories like this:

function loadStories() {
	const req = require.context('../components', true, /story.js$/);
	req.keys().forEach(filename => req(filename));
}
configure(loadStories, module);

and each story.js file has a respective sass or css file that is imported into it (for the purpose of story specific styles that are outside of the component that story.js would import, to display:

import './story.sass';

I have about 4 stories right now and this is the source of each story iframe... loading every stylesheet:

screen shot 2017-03-21 at 9 56 22 am

Is this normal behavior...?

--

demo

https://github.com/moimikey/729-single-stories-pulling-all-stylesheets

@moimikey
Copy link
Contributor Author

also, webpack is emitting all of those stories, so i'm wondering if it's how i've configured webpack?

screen shot 2017-03-21 at 11 27 10 am

@moimikey
Copy link
Contributor Author

moimikey commented Mar 21, 2017

i tried to use a decorator also, and this ended up being half broken, as i was able to at least isolate the additional stylesheet to the story, but as i traversed through others, those styles would break unless i did a hard refresh.

.addDecorator((getStory) => {
  require('./story.sass');
  return getStory();
})

@moimikey
Copy link
Contributor Author

@arunoda @mnmtanish

@ndelangen
Copy link
Member

Interesting!, do you have a repo that demonstrates this?

@moimikey
Copy link
Contributor Author

@ndelangen will make one shortly

@moimikey
Copy link
Contributor Author

@ndelangen https://github.com/moimikey/729-single-stories-pulling-all-stylesheets

@ndelangen ndelangen self-assigned this Mar 28, 2017
@ndelangen ndelangen added the bug label Apr 5, 2017
@ndelangen
Copy link
Member

Still trying to find the time to check this out..

@moimikey
Copy link
Contributor Author

moimikey commented Apr 5, 2017

thanks ;D it's a weird one. i've given it a look, but i'm uncertain where to start.

@ndelangen
Copy link
Member

So what I think is happenings is all the CSS files are getting picked up by webpack style-loader and just injected into the head. wether they are used or not.

@ndelangen
Copy link
Member

ndelangen commented Apr 5, 2017

But how to fix?

What I've done for my CSS is use CSS-modules. and import the generated classnames into my JS. Even if all CSS is injected together into the head, it doesn't matter, because it's guaranteed to be unique classes/selectors.

It doesn't really solve the exact problem you're having. But I think this is the intended behaviour of the style-loader.

@moimikey
Copy link
Contributor Author

moimikey commented Apr 5, 2017

this is true. i (and my company) are scoping with css, but we're doing a huge code redevelopment, so we have shared styles, thus a combination of styleName and className. so what ends up affecting storybook are those "outsider" css files.

i'll look into storybook code again tonight after i've had some beers and figure out how to perhaps solve this, implementation wise.

@thani-sh
Copy link
Contributor

thani-sh commented Apr 8, 2017

@moimikey for your problem I guess you have to skip the style-loader and load css files manually with a decorator. Something like this perhaps:

.addDecorator(getStory => (
  <div>
    <link ... />
    {getStory()}
  </div>
))

@moimikey
Copy link
Contributor Author

moimikey commented Apr 9, 2017

Wow .... that never crossed my mind... I'll try that.

@moimikey
Copy link
Contributor Author

moimikey commented Apr 9, 2017

ick but then again, using sass... x_x. i even tried an inline require. but i didn't have much luck. that would be an excellent solution for straight up css though :)

@moimikey
Copy link
Contributor Author

moimikey commented Apr 9, 2017

so @mnmtanish. thank you for your guidance. i've solved my issue with your inspiration:

.addDecorator((getStory) => {
  require('./story.sass');
  return getStory();
})

@moimikey
Copy link
Contributor Author

moimikey commented Apr 9, 2017

mmm so the only issue then now is that when you navigate from story to story, it's stacking the styles :(

@thani-sh
Copy link
Contributor

thani-sh commented Apr 9, 2017

Maybe the style loader can be configured to insert it somewhere other than HEAD so it can be removed. I haven't done this so not sure if it's even possible. Check these out.

@jrmyio
Copy link

jrmyio commented Apr 13, 2017

Isn't it the responsibility of react storybook to load the component in complete isolation and thus only have JavaScript/CSS ran that is related to the current selected story?

Is this related to #686?

@moimikey
Copy link
Contributor Author

@ConneXNL yes. good point. this is true... ;X

@moimikey
Copy link
Contributor Author

@mnmtanish my next response of course leads to another issue, insertAt might be useful, but it only available in the latest version of style-loader, which is not possible to use with storybook, because it uses loader-utils@1.x. storybook is still using 0.x. the latest possible version we can use is style-loader@0.13.1 :(...

@ndelangen
Copy link
Member

Hey @moimikey maybe you can try the approach the last mentioned with the alpha release?

@jrmyio
Copy link

jrmyio commented Jun 8, 2017

Isn't it better/safer to create a new iframe element when switching stories?

@ndelangen
Copy link
Member

That'd be safe, and also bad for performance.

The new iframe would have to parse the javascript, parse the CSS, connect to the postmessage-channel, reconnect to the websocket, and probably more stuff.

Maybe removing <style> elements on story-switch is enough to resolve this issue?

Of course there are no global styles and everything is properly namespaced, this wouldn't be an issue outright. Wishful thinking I guess. 😃

@ndelangen
Copy link
Member

If someone can prove the above statements are false or can demonstrate there's no negative consequences, I'm all ears!

@jrmyio
Copy link

jrmyio commented Jun 8, 2017

I did some tests today. The decorator pattern worked well so that styles are only injected once you switch to the story. However, I had the same issue where the styles aren't removed when switching stories.

I played around with removing styles in a decorator but it seems the required style is only applied once. Is it possible to re-trigger a require()? I tried using singleton: false but that didn't fix the problem.

@ndelangen
Copy link
Member

I'm almost hesitant to even suggest this, but you could try busting the webpack cache:
https://webpack.github.io/docs/api-in-modules.html#advanced

This is webpack 1 docs, but might still work.

@pocka
Copy link
Contributor

pocka commented Jan 23, 2020

@ndelangen
Yes 😃

By the way, what do you think about letting users to write raw-webpack-query? (like !to-string-loader!...) I think we need a lot of black magic in the addon code if we want to get rid of this...

@ndelangen
Copy link
Member

I think we support babel-macros by default, so users could be using macro-preval to inject file-contents into the bundle as well?

@pocka
Copy link
Contributor

pocka commented Jan 23, 2020

I didn't know that, I'll take a look at it soon!

@valec-openmind
Copy link

Hi there, i'm experiencing the same issue.

@pocka
Copy link
Contributor

pocka commented Feb 21, 2020

For anyone facing the issue, please try this workaround. (It needs a little deep webpack knowledge though).


@ndelangen I took a look at the macro but what it enables is "loading CSS files from file system", right? I think many users want to import SASS, Less, Stylus, CSS files with PostCSS, or etc, so that approach wouldn't satisfy the needs. My current idea is CSSResource addon adding a rule that imports CSS file with to-string-loader (or file-loader) so that the user could import a CSS file then use it for the addon.

// adding a rule like this
{
  test: /\.css$/,
  resourceQuery: /cssresources/,
  use: ['to-string-loader', 'css-loader', 'postcss-loader']
}

For pre-processors, an option to customize test and use is also needed. It's possible to pick a rule for style file then modify it with oneOf but it would be so complicated...

What do you think?

@ndelangen
Copy link
Member

@pocka yeah that sounds like an interesting concept!

@gptt916
Copy link

gptt916 commented Mar 16, 2020

Hi, wondering if this is still being worked on? I am also experiencing the issue, I am aware of the work around but would like to know if a fix will be available any time soon.

@meantsaki
Copy link

Hi, can you provide a example of the workaround with a Vue Story.?

@tylersticka
Copy link

As far as I can tell, the workaround still results in stylesheets stacking after initial view, at least if you're using the Docs add-on. 😕

@rhendric
Copy link

With no disrespect intended, I find @pocka's addon approach rather lacking, as it doesn't isolate styles that have been imported in component files as opposed to in the story files, which I think is the more common pattern. My personal desire—I suspect this may be shared by others in this thread—is to be able to have an import './Button.css' inside of Button.jsx that only gets used in story files where Button.jsx is imported. Per-story styling, in the manner that @pocka has provided, isn't nearly as important to me as making sure that no component that doesn't itself import Button (directly or indirectly) isn't affected by any CSS rules from Button.css. (The concern here is wanting to make sure that OtherWidget.css, say, isn't lacking some rules that inadvertently ended up in Button.css instead—maybe they got overlooked in a refactoring or something—and missing it because the stories for OtherWidget get all the statically-imported CSS of the entire app, so OtherWidget still looks fine in Storybook.)

What I think I would do instead is change all CSS loaders to inject with lazyStyleTag, and then use webpack API to generate a new module that groups the CSS modules by the story files that ultimately require them and listens to story change events to turn the appropriate modules on and off.
Has this approach already been considered and discarded, or are there any issues with it that you see now? I think I can do it all in a Storybook addon, but it might be cleaner if integrated into Storybook as a core feature.

@bennypowers
Copy link
Contributor

If you want strong style encapsulation, browsers ship with it. At the risk of exposing my own ignorance here, I'm still not sure why all this userland JavaScript (with its associated complexity costs) is necessary to accomplish something that's built-in and ready to go.

Why not just render each story's DOM into a shadow root with something like this

customElements.define('encapsulated-story', class EncapsulatedStory extends HTMLElement {
  constructor() {
    super();
    this.attachShadow({ mode: 'open' });
  }

  /* not sure why we'd need this getter, but let's say */
  get storyHTML() {
    return this.shadowRoot.innerHTML;
  }

  set storyHTML(string) {
    this.shadowRoot.innerHTML = storyHTML;
  }
});

and whenever the story changes

encapsulatedStory.storyHTML = theStoryDOMStringWithAllStyleTags;

and done? Here, theStoryDOMStringWithAllStyleTags is just the concatenation of a story's HTML with all associated styles concated as inline <style> tags. Story's could style the host element with the :host selector, as normal.

That's a bare-minumum starting point, which could be built on later on perhaps with some library code, but at least it will accomplish the goal of strong style encapsulation without the need for all these new proposed APIs.

@rhendric
Copy link

How does that work with webpack, though? Webpack packages everything up into a JavaScript bundle, not a DOM string; and the way webpack is currently configured, it packages all stories up into a single bundle. I don't think a shadow root helps when the JavaScript being (hot-re)loaded inserts styles directly into the document's head. You need to do some hairy webpack configuration one way or another to change that.

Using the shadow DOM to completely isolate each story would also mean replicating the style tags shared by many stories into each one; using a shared style as bundled by webpack will be more efficient. Maybe not enough to make a huge difference, but possibly enough to offset the benefits, if there are any, of using the shadow DOM instead of using lazyStyleTag (which I think is the only piece of complexity that shadow roots would save you).

@danielo515
Copy link

I am also interested on seeing this fixed sooner or later.

@matthias-ccri
Copy link
Contributor

That'd be safe, and also bad for performance.

The new iframe would have to parse the javascript, parse the CSS, connect to the postmessage-channel, reconnect to the websocket, and probably more stuff.

@ndelangen sorry to quote you from 2017, but is this still your perspective, that reloading an iframe is too expensive? Browsers do this type of thing constantly; they're probably very optimized for it. In this case it would be even faster than a regular page load, since there are no network requests involved.

To me, the benefit outweighs the cost, because I would very much prefer a fresh iframe for each story. I would tolerate a delay of as much as 600ms for such a luxury.

(My use case is that I'm trying to render some legacy angularjs components in storybook, and let's just say that the components aren't very pure. They're very stateful; they have side effects and make use of angularjs services which are also very stateful. Things break in unexpected ways.)

One idea for an API surface is to configure storybook in .storybook/manager.js.

addons.setConfig({
  refreshBetweenStories: true,
})

This could be considered a UI setting if you tilt your head the right way.

There would be no runtime cost for people who do not enable this flag, and for those who do enable it, they really need it, so any such delay would be tolerable.

@shilman
Copy link
Member

shilman commented Oct 1, 2020

If you want this fixed, please upvote by adding a 👍 to the issue description. We use this to help prioritize!

@joneldiablo
Copy link

.addDecorator((getStory) => {
require('./story.sass');
return getStory();
})

HI!!!!
where can I put this one?!!!

@ashleynolan
Copy link

We're seeing the same issue on an instance of Storybook running with Vue.

We have a component mono-repo and where components are shared across larger components (for example, a button component, which is then used inside a modal component and a banner component) the styles from both components are loaded in and they conflict (even though they are using CSS Modules to scope the CSS).

As we have visual regression tests running against Storybook, it means that visually our components look different in Storybook to how they do when properly built as a library component. Being able to run these components in true isolation from the other component styles would solve this issue.

Are there any plans to work on this use-case? We'd be happy to help find a solution to this and contribute back, but not really sure where to start so would need some help to do so.

@ndelangen
Copy link
Member

ndelangen commented Aug 2, 2021

and they conflict (even though they are using CSS Modules to scope the CSS).

This confuses me, can you explain?

@helloanil
Copy link

Is there any working solution for this, for Storybook 6?

@bennypowers
Copy link
Contributor

Is there any working solution for this, for Storybook 6?

try this

import { render, html } from 'lit-html';

let id = 1;


function shadowDecorator(storyFn) {
  // generate a tagname. maybe use storyname here?
  const tagName = `shadow-story-${id++}`;
  // define a custom element which renders the story to its shadow root
  // optionally, include a css reset here?
  customElements.define(tagName, class ShadowStory extends HTMLElement {
    constructor() {
      super();
      this.attachShadow({ mode: 'open' });
      // season to taste, depending on sb flavour
      render(storyFn(), this.shadowRoot);
    }
  });
  return document.createElement(tagName);
}

export const decorators = [shadowDecorator];

@shilman
Copy link
Member

shilman commented Sep 20, 2021

closing this as duplicate to #16016 .. let's continue discussion there!

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

No branches or pull requests