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

Added an upgrade mode to getstorybook #1146

Merged
merged 2 commits into from
May 30, 2017
Merged

Added an upgrade mode to getstorybook #1146

merged 2 commits into from
May 30, 2017

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented May 29, 2017

Issue: You need to apply codemod and update your package.json to upgrade to v3

What I did

Added a mode to getstorybook[1] that picks up when it is run in an pre-v3 project and updates all package references in package.json and using the codemod.

[1] Should we rename the command to storybook [install] now as well?

How to test

Grab a 2.0 project, run getstorybook (after npm link-ing of course). It should have correctly updated all references to the packages in both package.json and JS files.

@@ -7,7 +7,7 @@
"type": "git",
"url": "git+https://github.com/storybooks/storybook.git"
},
"main": "src/index.js",
"main": "dist/index.js",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if the old main field was a mistake or if I'm missing something

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This package is never ever required by anything. This explains why this bug was never found.

@tmeasday
Copy link
Member Author

I've tested this on a CRA project on OSX. It would be great if we could test it on:

(A) React Native
(B) Windows (does getstorybook work on windows?)

}

function updateSourceCode() {
const jscodeshiftPath = path.dirname(require.resolve('jscodeshift'));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a reliable way to get the path to the compiled package? jscodeshift doesn't appear to offer its commandline functionality via an API

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you found this method to be unreliable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but it feels a little hacky. For instance it hard codes the path of the binary into this file (rather than npm run jscodeshift which uses the bin field of jscodeshift's package.json).

@codecov
Copy link

codecov bot commented May 29, 2017

Codecov Report

Merging #1146 into master will decrease coverage by 0.03%.
The diff coverage is 5.26%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1146      +/-   ##
==========================================
- Coverage   12.75%   12.72%   -0.04%     
==========================================
  Files         199      199              
  Lines        4571     4582      +11     
  Branches      724      727       +3     
==========================================
  Hits          583      583              
- Misses       3347     3356       +9     
- Partials      641      643       +2
Impacted Files Coverage Δ
lib/cli/lib/project_types.js 0% <ø> (ø) ⬆️
lib/cli/lib/detect.js 0% <0%> (ø) ⬆️
lib/cli/bin/generate.js 0% <0%> (ø) ⬆️
lib/codemod/src/index.js 0% <0%> (ø) ⬆️
...codemod/src/transforms/update-organisation-name.js 40.62% <12.5%> (-2.71%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe882f9...b3c47e3. Read the comment docs.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tmeasday how do I properly link? What I did:

npm run bootstrap
cd lib/cli
npm link
cd path/to/testapp_with_storybook2
getstorybook

Then I get the following error:

MBP:testapp shilman$ getstorybook

 getstorybook - the simplest way to add a storybook to your project. 

 • Detecting project type. ✓
module.js:471
    throw err;
    ^

Error: Cannot find module './transforms/update-organisation-name'
    at Function.Module._resolveFilename (module.js:469:15)
    at Function.Module._load (module.js:417:25)
    at Module.require (module.js:497:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/Users/shilman/projects/storybook/new/storybook/lib/codemod/dist/index.js:7:31)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)

I get the same error when I try to run ../../path/to/storybook/lib/cli/bin/generate.js directly

@tmeasday
Copy link
Member Author

You may need to npm install inside the cli sir?

@shilman
Copy link
Member

shilman commented May 29, 2017

@tmeasday bootstrap already does the install? Even when I manually install inside the clr, I still get the same error.

@tmeasday
Copy link
Member Author

tmeasday commented May 29, 2017

Hmm, I think the culprit is this line: https://github.com/storybooks/storybook/blob/master/lib/codemod/package.json#L15 which does something a bit weird. (Fill me in @ndelangen? It seems weird/broken to have the compiled behaviour/file locations be different from the uncompiled ones)

[I'm not sure why but for some combination of commands it actually didn't move the file from the dist/transforms directory, and thus the PR originally worked for me]

I can update the PR to remove that weirdness, but let's find out why it's there first.

@shilman
Copy link
Member

shilman commented May 29, 2017

@tmeasday I suspect it was just to make this upgrade instruction shorter for users:

./node_modules/.bin/jscodeshift -t ./node_modules/@storybook/codemod/dist/update-organisation-name.js . --ignore-pattern "node_modules|dist"

@tmeasday tmeasday force-pushed the cli-upgrade-mode branch from 20aa565 to f1c4474 Compare May 29, 2017 11:10
@tmeasday
Copy link
Member Author

Ok, I'll make the change now and @ndelangen can approve that specific change.

"dependencies": {
"jscodeshift": "^0.3.30"
},
"scripts": {
"prepublish": "node ../../scripts/prepublish.js && mv dist/transforms/* dist"
"prepublish": "node ../../scripts/prepublish.js"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ndelangen you should perhaps check this change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this to shorten the paths, if this is removed the manual needs to be updated

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the README for the package, was there anything else that needed to change?

@tmeasday tmeasday force-pushed the cli-upgrade-mode branch from 1b75f8b to b534551 Compare May 29, 2017 11:18
This allows us to go from 2.x -> 3.0

A future version of storybook may include a version # that allows us to be more subtle, but we don't need it yet.
@tmeasday tmeasday force-pushed the cli-upgrade-mode branch from b534551 to 104ca51 Compare May 30, 2017 00:22
@tmeasday
Copy link
Member Author

FTR I think this worked for @shilman, he needs to approve the review now I guess.

@shilman shilman merged commit 7ac299f into master May 30, 2017
@Hypnosphi Hypnosphi deleted the cli-upgrade-mode branch August 17, 2017 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants