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

sb migrate doesn't work with npx #7602

Closed
Hypnosphi opened this issue Jul 29, 2019 · 22 comments
Closed

sb migrate doesn't work with npx #7602

Hypnosphi opened this issue Jul 29, 2019 · 22 comments

Comments

@Hypnosphi
Copy link
Member

Hypnosphi commented Jul 29, 2019

$ npx -p @storybook/cli@5.2.0-beta.17 sb migrate storiesof-to-csf --glob "components/**/*.examples.js"
[BABEL] Note: The code generator has deoptimised the styling of /Users/jetbrains/.npm/_npx/50842/lib/node_modules/@storybook/cli/node_modules/lodash/lodash.js as it exceeds the max of 500KB.
=> Applying storiesof-to-csf: 117 files
yarn run v1.17.3
error Command "jscodeshift" not found.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

$ npx -v
6.9.0
$ node -v
v10.16.0
$ npm -v
6.9.0


@shilman
Copy link
Member

shilman commented Aug 1, 2019

Jiminy cricket!! I just released https://github.com/storybookjs/storybook/releases/tag/v5.2.0-beta.20 containing PR #7649 that references this issue. Upgrade today to try it out!

You can find this prerelease on the @next NPM tag.

Closing this issue. Please re-open if you think there's still more to do.

@shilman shilman closed this as completed Aug 1, 2019
@Hypnosphi
Copy link
Member Author

Hypnosphi commented Aug 1, 2019

Still doesn't work:

$ npx -p @storybook/cli@5.2.0-beta.20 sb migrate storiesof-to-csf --glob "components/**/*.examples.js"
[BABEL] Note: The code generator has deoptimised the styling of /Users/jetbrains/.npm/_npx/84370/lib/node_modules/@storybook/cli/node_modules/lodash/lodash.js as it exceeds the max of 500KB.
=> Applying storiesof-to-csf: 117 files
Processing 117 files... 
Spawning 7 workers...
Sending 17 files to free worker...
Sending 17 files to free worker...
Sending 17 files to free worker...
Sending 17 files to free worker...
Sending 17 files to free worker...
Sending 17 files to free worker...
Sending 15 files to free worker...
internal/modules/cjs/loader.js:638
    throw err;
    ^

Error: Cannot find module 'core-js/modules/es6.string.iterator'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:636:15)
    at Function.Module._load (internal/modules/cjs/loader.js:562:25)
    at Module.require (internal/modules/cjs/loader.js:690:17)
    at require (internal/modules/cjs/helpers.js:25:18)
    at Object.<anonymous> (/Users/jetbrains/.npm/_npx/84370/lib/node_modules/@storybook/cli/node_modules/@storybook/codemod/dist/transforms/storiesof-to-csf.js:3:1)
    at Module._compile (internal/modules/cjs/loader.js:776:30)
    at Module._compile (/Users/jetbrains/IdeaProjects/ring-ui/node_modules/pirates/lib/index.js:99:24)
    at Module._extensions..js (internal/modules/cjs/loader.js:787:10)
    at Object.newLoader [as .js] (/Users/jetbrains/IdeaProjects/ring-ui/node_modules/pirates/lib/index.js:104:7)
    at Module.load (internal/modules/cjs/loader.js:653:32)

@Hypnosphi Hypnosphi reopened this Aug 1, 2019
@Hypnosphi
Copy link
Member Author

Hypnosphi commented Aug 1, 2019

Oddly enough, /Users/jetbrains/.npm/_npx/84370/lib/node_modules/@storybook/cli/node_modules/@storybook/codemod/dist/transforms/storiesof-to-csf.js:3 doesn't actually import 'core-js/modules/es6.string.iterator', it imports 'core-js/modules/es.array.iterator' which exists in core-js 3. Some caching issue maybe?

@shilman
Copy link
Member

shilman commented Aug 1, 2019

It's working for me. Will investigate if other people are seeing this problem.

@stale stale bot added the inactive label Aug 22, 2019
@stale stale bot removed the inactive label Sep 17, 2019
@Hypnosphi
Copy link
Member Author

My workaround was to run the command on a parent level instead of project directory. Looks like it interfered with my local packages in some way

@storybookjs storybookjs deleted a comment from stale bot Sep 17, 2019
@shilman shilman modified the milestones: 5.2.0, 5.2.x Sep 23, 2019
@stale
Copy link

stale bot commented Oct 14, 2019

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label Oct 14, 2019
@fabb
Copy link

fabb commented Oct 23, 2019

I tried running it from the parent directory, it did not fail anymore at the babel issue but still failed at a later point at Error: Cannot find module 'react'. But that's the same error as when running a globally installed sb.

@stale stale bot removed the inactive label Oct 23, 2019
@kevinkuebler
Copy link

kevinkuebler commented Oct 24, 2019

I just tried to run the migration and I'm getting the same error as @fabb. Our stories are all written in TS in separate files in a stories directory, so I ran (from the project directory):
npx -p @storybook/cli sb migrate storiesof-to-csf --glob "stories/*.tsx"

The error returned is:

internal/modules/cjs/loader.js:638
    throw err;
    ^

Error: Cannot find module 'react'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:636:15)
    at Function.Module._load (internal/modules/cjs/loader.js:562:25)
    at Module.require (internal/modules/cjs/loader.js:690:17)
    at require (internal/modules/cjs/helpers.js:25:18)
    at Object.require (C:\Users\kkuebler\AppData\Roaming\npm-cache\_npx\23068\node_modules\@storybook\cli\node_modules\@storybook\router\dist/router.js:36:37)
    at Module._compile (internal/modules/cjs/loader.js:776:30)
    at Module._compile (C:\Dev\Spry-React\node_modules\pirates\lib\index.js:99:24)
    at Module._extensions..js (internal/modules/cjs/loader.js:787:10)
    at Object.newLoader [as .js] (C:\Dev\Spry-React\node_modules\pirates\lib\index.js:104:7)
    at Module.load (internal/modules/cjs/loader.js:653:32)

Any ideas? We're really excited to get everything converted to CSF and to start using the new Docs addon. Thanks!

Looks like maybe this is a duplicate of #8209?

@kevinkuebler
Copy link

Based on the info in #8209 I ran the command again using version 5.2.0, and it was able to run. Unfortunately most of the conversions failed, it looks like due to unrecognized TypeScript syntax. For example any story that had a declaration like:
interface State { foo: string }
failed with this error:
ERR stories/alphaBar.tsx Transformation error (Unexpected reserved word 'interface' (14:0))

Are there known issues like this around stories written in TS?

@shilman
Copy link
Member

shilman commented Oct 25, 2019

@kevinkuebler there's this one: #8091

@kevinkuebler
Copy link

kevinkuebler commented Oct 25, 2019

@shilman Thanks, I didn't know about the parser switch. So, it looks like I should have been using --parser=tsx (assuming that bug wasn't an issue)?

In the meantime, I think I'll try temporarily commenting out all of the interface definitions in our stories and see how far the migration tool is able to get then. If that doesn't work, we have a small enough number of stories that I may just convert them by hand.

@shilman
Copy link
Member

shilman commented Oct 25, 2019

@kevinkuebler there's a good chance the migrate script won't work with typescript even once that bug is fixed. it's doing an analysis of the parsed abstract syntax tree (AST) to do the source-to-source transformation, and that AST will be different for typescript. however the code is only written to handle the JS AST. It's still worth a try, because there's probably a high degree of overlap, and the codemod can save you time, even though it might not get 100% of the cases. But don't set your hopes too high.

@kevinkuebler
Copy link

kevinkuebler commented Oct 25, 2019

@shilman Got it, thanks for the info! I'll try to touch up our stories a bit (like commenting out the interfaces) and see if that gets us most of the way there.

UPDATE: Just commenting out the interfaces isn't enough, usually because we have a class component defined in the story that uses the TS generic syntax (e.g. ExampleComponent extends React.Component<Props, State> {...}. So it looks like I'll mostly be converting by hand. The conversion is pretty straight-forward though, so it shouldn't be too bad.

@fabb
Copy link

fabb commented Oct 25, 2019

For me the transformation of typescript stories worked fine, since I don‘t have any interfaces or the like defined in these files.

I needed to install the storybook cli and react globally to make the above mentioned errors disappear. Unfortunately no npx.

@shilman
Copy link
Member

shilman commented Oct 26, 2019

@kevinkuebler i don't think that's something i'd go out of my way to support, but if you or anybody else sees super common TS patterns that break storiesof-to-csf, i'd be happy to see if i can add something quick to account for those.

@kevinkuebler
Copy link

@shilman No problem, I understand. Just to add a bit more context, here's why our stories are written the way they are. I'm curious to know if anybody else does this or if we were maybe not using Storybook in a recommended way when we started writing these.

Let's say we have a fairly simple, fully controlled React component named TextField which is pretty much what you'd expect, a standard component for rendering a text <input> element. Our feeling was, in order to make the story useful, you should be able to interact with the component in Storybook. And so to do that, we would write a component much like we would in our actual application, which would hold the state and have the onchange handler. So our story file, written in TS, would look something like this:

import * as React from 'react';
import { storiesOf } from '@storybook/react';

interface State {
  textValue: string;
}

class TextFieldStory extends React.Component<{}, State> {
  constructor(props: {}) {
    super(props);
    this.state = {
      textValue: ''
    }
  }

  render() {
    return <TextField value={this.state.textValue} onChange={this.textValueChanged} />
  }

  private textValueChanged = (value: string) => {
    this.setState( { textValue: value } );
  }
}

storiesOf('TextField', module).add('simple', () => {
  return <TextFieldStory />
})

We have quite a few stories written this way. Usually it's because they're illustrating how an application component would use the actual component that the story is ultimately about, in regards to managing state, etc. I realize that may not be how Storybook is intended to be used, since the component being passed to Storybook is not the actual component that we're trying to document.

That issue aside, the ideal scenario for the migration would be that it basically ignores everything except the call to the storiesOf api and just converts that. But I realize it needs to parse the code in order to do that, so that's simpler said than done. 😄

@shilman
Copy link
Member

shilman commented Oct 28, 2019

@kevinkuebler maybe @Hypnosphi has more to say about this, but storybook now supports hooks out of the box: both react hooks and also a framework independent hook implementation. i'd guess that would be a much more compact/convenient way to do the same thing.

https://github.com/storybookjs/storybook/blob/next/examples/official-storybook/stories/hooks.stories.js

@kevinkuebler
Copy link

@shilman Oh, for sure. We haven't made the move to hooks yet in our production code, but it's on the horizon. A lot of the stories we're converting were written before hooks were even a thing. For something simple like this example, I would definitely do what's shown in that example you linked when writing new stories. Some of our more complex components though would probably still require a separate story component to wrap the component we're documenting in Storybook (although in some cases we may be able to make better use of decorators to accomplish that).

Anyway, I don't want to wander too far from the original point of this issue, which was the migration. 😄 And since the migration tool doesn't TS syntax, I'm ok with manually converting our stories. Thanks again!

@elmpp
Copy link

elmpp commented Nov 6, 2019

Had to strip out the typescript but worked ok (globally install sb and react however)

@stale
Copy link

stale bot commented Nov 27, 2019

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label Nov 27, 2019
@shilman shilman added the todo label Dec 11, 2019
@stale stale bot removed the inactive label Dec 11, 2019
@shilman shilman modified the milestones: 5.2.x, 5.3.x Jan 11, 2020
@shilman shilman removed this from the 5.3.x milestone Jul 30, 2020
@naasimshareef
Copy link

naasimshareef commented Nov 27, 2020

For me, the error was due to missing storybook cli. This command worked like a charm:
npx -p @storybook/cli sb upgrade

@vanessayuenn
Copy link
Contributor

Hi! I am going through and cleaning up old issues in the repo. We just shipped a ton of improvements in Storybook 7.0, and I suspect this bug might be fixed already. Can you please try it out? If this issue persists with the latest version, please open a new bug report with a 7.x reproduction. Thank you so much! 🙏🏼

@vanessayuenn vanessayuenn closed this as not planned Won't fix, can't repro, duplicate, stale May 12, 2023
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

7 participants