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

Storybook 5.2 with CSF: __esModules story for every component #7710

Closed
chasemccoy opened this issue Aug 7, 2019 · 25 comments
Closed

Storybook 5.2 with CSF: __esModules story for every component #7710

chasemccoy opened this issue Aug 7, 2019 · 25 comments
Milestone

Comments

@chasemccoy
Copy link

@chasemccoy chasemccoy commented Aug 7, 2019

Describe the bug
I installed the beta of Storybook 5.2 and ran the codmod to update our stories. Everything seemed to work just fine, except all of my components now have a story called __esModule. Clicking on this story displays an error message: Attempted to set a non-object key in a WeakMap (screenshots below)

Screenshots
Screen Shot 2019-08-07 at 1 46 22 PM

Storybook config file:

import { configure, addDecorator, addParameters } from '@storybook/react';
import { jsxDecorator } from 'storybook-addon-jsx';
import { withA11y } from '@storybook/addon-a11y';
import { withKnobs } from '@storybook/addon-knobs';
import theme from './theme';

import icons from '!!raw-loader!../dist/icons.svg'

addDecorator(jsxDecorator);
addDecorator(withA11y);
addDecorator(withKnobs);

const req = require.context("../src", true, /.stories.js$/);

function loadStories() {
  const div = document.createElement('div');
  div.style.cssText = 'display:none;';
  div.innerHTML = icons;
  document.body.appendChild(div);
  return require.context('../src', true, /\.stories\.js$/)
}

addParameters({
  options: {
    theme: theme
  }
});

configure(loadStories(), module);

System:

  System:
    OS: macOS 10.14.5
    CPU: (8) x64 Intel(R) Core(TM) i7-6820HQ CPU @ 2.70GHz
  Binaries:
    Node: 10.15.1 - ~/.nvm/versions/node/v10.15.1/bin/node
    Yarn: 1.16.0 - ~/.yarn/bin/yarn
    npm: 6.9.0 - ~/.nvm/versions/node/v10.15.1/bin/npm
  Browsers:
    Chrome: 75.0.3770.142
    Firefox: 64.0
    Safari: 12.1.1
  npmPackages:
    @storybook/addon-a11y: ^5.2.0-beta.24 => 5.2.0-beta.24 
    @storybook/addon-knobs: ^5.2.0-beta.24 => 5.2.0-beta.24 
    @storybook/addon-viewport: ^5.2.0-beta.24 => 5.2.0-beta.24 
    @storybook/react: ^5.2.0-beta.24 => 5.2.0-beta.24 
    @storybook/theming: ^5.2.0-beta.24 => 5.2.0-beta.24 
@jnielson94

This comment has been minimized.

Copy link

@jnielson94 jnielson94 commented Aug 7, 2019

Something that might help diagnose the issue further - do you have any custom babel/webpack config running? It sounds like you might have a custom babel config getting picked up which might be configured incorrectly for storybook?

@chasemccoy

This comment has been minimized.

Copy link
Author

@chasemccoy chasemccoy commented Aug 7, 2019

@jnielson94 I do have a custom Babel config:

const BABEL_ENV = process.env.BABEL_ENV;

module.exports = () => ({
  plugins: [
    "babel-plugin-styled-components",
    "polished",
    "flow-react-proptypes",

    // Stage 3
    "@babel/plugin-syntax-dynamic-import",
    ["@babel/plugin-proposal-class-properties", { loose: true }]
  ],
  presets: [
    [
      "@babel/preset-env",
      {
        loose: true,
        modules: BABEL_ENV === "es" ? false : "commonjs",
        targets: {
          browsers: [
            "last 2 Firefox versions",
            "last 2 Chrome versions",
            "Safari >= 8",
            "last 2 Edge versions",
            "ie > 10"
          ]
        }
      }
    ],
    "@babel/react",
    "@babel/flow"
  ]
});
@jnielson94

This comment has been minimized.

Copy link

@jnielson94 jnielson94 commented Aug 7, 2019

To verify another thing, it looks like you're still using the storiesOf api, correct? Or did you switch to the new CSF?

@chasemccoy

This comment has been minimized.

Copy link
Author

@chasemccoy chasemccoy commented Aug 7, 2019

@jnielson94 Nope, I switched to CSF using the provided code-mod.

@jnielson94

This comment has been minimized.

Copy link

@jnielson94 jnielson94 commented Aug 7, 2019

Ah, just noticed you mentioned that in the initial description. Are you running storybook with a BABEL_ENV defined? If not, it looks like it'll default to commonjs, which would have babel adding something like:

Object.defineProperty(exports, "__esModule", {
  value: true
});

To each file, which would explain why the story exists (since in CSF the export would get picked up as a story) :) Does it go away if you switch your BABEL_ENV to es (or adjust your config)?

@chasemccoy

This comment has been minimized.

Copy link
Author

@chasemccoy chasemccoy commented Aug 7, 2019

@jnielson94 That fixed it! Worked like a charm. This could be good to document somewhere maybe? At least in the migration guide. I definitely assumed it was a bug when I first upgraded.

@jnielson94

This comment has been minimized.

Copy link

@jnielson94 jnielson94 commented Aug 7, 2019

I don't know the documentation well enough to know where it would make sense to note it - @shilman might have a better idea?

@chasemccoy

This comment has been minimized.

Copy link
Author

@chasemccoy chasemccoy commented Aug 7, 2019

@jnielson94 While I've got you here, I had another question about CSF... it seems as if this doesn't work:

Component.story = {
  name: "Category/Example"
};

I used to be able to nest stories into categories using this syntax, but it doesn't seem to work now. Is this a bug or expected?

@jnielson94

This comment has been minimized.

Copy link

@jnielson94 jnielson94 commented Aug 7, 2019

I'm not sure if that's expected or a bug, I know that I ran into that when I tried CSF on our codebase. Using nesting like that definitely works in the default export name (storiesOf equivalent), but I don't know if that was a decision that was made to not allow stories to nest themselves...

@chasemccoy

This comment has been minimized.

Copy link
Author

@chasemccoy chasemccoy commented Aug 7, 2019

@jnielson94 Good to know 👍 I know it can be done with the default export, but only one of those is allowed per-file, so we couldn't have multiple nested categories without making multiple component.stories.js files for each component.

@jnielson94

This comment has been minimized.

Copy link

@jnielson94 jnielson94 commented Aug 7, 2019

Yep, that's where my experimentation landed me as well. We decided that it'd probably be useful to break them up anyway to have smaller files to deal with, so we went ahead and did that (on 5.1 actually). I'd be curious to know the official stance/reason on stories nesting themselves though :)

@chasemccoy

This comment has been minimized.

Copy link
Author

@chasemccoy chasemccoy commented Aug 7, 2019

@jnielson94 Cool. I will open a separate issue to discuss that. Thanks for your help! Gonna go ahead and close this out.

@chasemccoy chasemccoy closed this Aug 7, 2019
@jnielson94

This comment has been minimized.

Copy link

@jnielson94 jnielson94 commented Aug 7, 2019

I'd be tempted to leave this one open to make sure the docs question gets addressed? Otherwise, glad to help!

@chasemccoy chasemccoy reopened this Aug 7, 2019
@shilman

This comment has been minimized.

Copy link
Member

@shilman shilman commented Aug 8, 2019

@chasemccoy @jnielson94 Looks like you guys sorted this out while I was sleeping.

Let's add this to the docs if it comes up again. CSF picks up all named exports. Perhaps it would be useful to ignore exports whose names start with underscore? That would be a breaking change, but I'd consider it in 6.0. What do you guys think.

Related feature is you can add includeStories/excludeStories rules to your default export. But your solution here is much better, since you'd need to add that to every file, which would suck.

Re: Story nesting, it works exactly as in storiesOf. So:

export default { title: 'Foo|Bar/baz' }
export const apple = () => ...
zoo.story = { name: 'apple/banana' }

Gets translated to:

storiesOf('Foo|Bar/baz').add('apple/banana', () => ...)

I don't expect this to change in future versions of CSF.

@chasemccoy

This comment has been minimized.

Copy link
Author

@chasemccoy chasemccoy commented Aug 8, 2019

@shilman In that example above, apple/banana won't get turned into a nested accordion called "apple" with a story called "banana" in it, will it? That's what I am trying to achieve. Having that happen in the default export means I can't have multiple of those in one file. I'd like to avoid having multiple story files per component.

@shilman

This comment has been minimized.

Copy link
Member

@shilman shilman commented Aug 8, 2019

@chasemccoy I see, that's interesting. Won't change in 5.2, but we could consider something like that for a future release. No promises! cc @tmeasday

@shilman shilman added this to the 5.2.0 milestone Aug 8, 2019
@chasemccoy

This comment has been minimized.

Copy link
Author

@chasemccoy chasemccoy commented Aug 8, 2019

@shilman Thanks so much! 🙏 Definitely not a deal breaker for me. My team and I are super excited about CSF.

@tmeasday

This comment has been minimized.

Copy link
Member

@tmeasday tmeasday commented Aug 8, 2019

I didn't even know we supported that in the .add() API 😬

I think it (a) probably breaks the assumptions made in CSF and (b) is rarely used enough that we shouldn't support it.

But correct me if I am wrong on (a) or (b).

@shilman

This comment has been minimized.

Copy link
Member

@shilman shilman commented Aug 8, 2019

@tmeasday We don't support that in the .add API. The story will just get the name 'apple/banana' which doesn't help @chasemccoy! 😁

@shilman

This comment has been minimized.

Copy link
Member

@shilman shilman commented Aug 8, 2019

Closing this for now, reply if there's more to resolve!

@shilman shilman closed this Aug 8, 2019
@chasemccoy

This comment has been minimized.

Copy link
Author

@chasemccoy chasemccoy commented Aug 8, 2019

Sorry, I think I explained this poorly. Here's the code we had before:

storiesOf("Checkbox", module)
  .add("Checked", () => <Checkbox checked={boolean("checked", true)} />)
  .add("Unchecked", () => <Checkbox checked={boolean("checked", false)} />);

storiesOf("Checkbox/Disabled", module)
  .add("Checked", () => (
    <Checkbox
      checked={boolean("checked", true)}
      disabled={boolean("disabled", true)}
    />
  ))
  .add("Unchecked", () => (
    <Checkbox
      checked={boolean("checked", false)}
      disabled={boolean("disabled", true)}
    />
  ));

and it gave us this:

Screen Shot 2019-08-07 at 10 22 08 PM

As far as I can tell, with CSF there is no way to replicate this without creating multiple files (so that you can have more default exports).

@chasemccoy

This comment has been minimized.

Copy link
Author

@chasemccoy chasemccoy commented Aug 8, 2019

What I suppose I expected when switching to CSF if that the default export title would be Checkbox, and then naming a story Disabled/Checked would allow me to get the same nesting I had before (but in a single file).

@chasemccoy

This comment has been minimized.

Copy link
Author

@chasemccoy chasemccoy commented Aug 8, 2019

So... I guess what I am saying is that the nesting separators seem to make more sense to me at the story level with CSF, instead of at the module level. If a module == a component in the mental model (at least this is our mental model), then it seems reasonable that you would have stories within that component that would have different nesting. But at the moment it seems nesting is only possible at the module level.

@shilman

This comment has been minimized.

Copy link
Member

@shilman shilman commented Aug 8, 2019

@chasemccoy I understood your request. The current structure is by design. Our intent is that each component will have its own file and will be placed in the user-specified position in the hierarchy. It's flexible, you can achieve what you're looking for by creating more files. But for now we're pretty sticking to the storiesOf structure, and we don't have any immediate plans to deviate from that.

That said, we're also receptive to feedback from users (like yourself) and partners (integrating with other tools), so the format will surely evolve. My point is that you shouldn't hold your breath on being able to nest in the way you requested. It complicates the format and I'm not sure whether we'll add it. And even if we did, it wouldn't make it in until 6.0, which will be awhile.

@chasemccoy

This comment has been minimized.

Copy link
Author

@chasemccoy chasemccoy commented Aug 8, 2019

@shilman Good to know! Thanks for walking me through it. Like I said, definitely not a deal breaker for us 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.