Fixes to build scripts for Windows. #2051

AaronFriel · 2017-10-14T17:45:32Z

Issue: On Windows platforms, the dev scripts in .\app\react and vue relied on three things that didn't work:

They assumed the parent project's dependencies were installed in the prepare script. This is fixed by adding prepare-root.
The environment variable DEV_BUILD is set in a *nix-ish way, which doesn't work in PowerShell.
The --exec parameter of nodemon used single quotes instead of double quotes.

What I did

I added a prepare-root script which builds the parent package, added that as an initial command to the dev script.

I added the cross-env dependency and used that to set the DEV_BUILD parameter.

I double quoted and escaped the --exec parameter.

I then tested these changes on a Linux environment. I used *nix style path separators in the prepare-root script.

codecov · 2017-10-14T17:52:58Z

Codecov Report

Merging #2051 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2051   +/-   ##
=======================================
  Coverage   21.34%   21.34%           
=======================================
  Files         262      262           
  Lines        5767     5767           
  Branches      692      703   +11     
=======================================
  Hits         1231     1231           
- Misses       4003     4008    +5     
+ Partials      533      528    -5

Impacted Files	Coverage Δ
app/react/src/server/config/babel.js	`0% <0%> (-100%)`	⬇️
app/react/src/server/babel_config.js	`0% <0%> (-77.42%)`	⬇️
app/react/src/server/utils.js	`0% <0%> (-53.58%)`	⬇️
app/react-native/src/preview/story_kind.js	`0% <0%> (ø)`	⬆️
lib/components/src/navigation/menu_link.js	`0% <0%> (ø)`	⬆️
addons/knobs/src/components/types/Object.js	`5.81% <0%> (ø)`	⬆️
lib/ui/src/modules/shortcuts/actions/shortcuts.js	`6.25% <0%> (ø)`	⬆️
lib/ui/src/modules/ui/configs/init_panels.js	`25% <0%> (ø)`	⬆️
...es__/update-addon-info/update-addon-info.output.js	`0% <0%> (ø)`	⬆️
lib/codemod/src/transforms/update-addon-info.js	`51.16% <0%> (ø)`	⬆️
... and 23 more

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 1575fae...bae95e3. Read the comment docs.

ndelangen · 2017-10-14T21:40:24Z

Would you care to review this @usulpro ?

Hypnosphi · 2017-10-16T10:33:27Z

app/react/package.json

@@ -18,7 +18,8 @@
    "url": "https://github.com/storybooks/storybook.git"
  },
  "scripts": {
-    "dev": "DEV_BUILD=1 nodemon --watch ./src --exec 'yarn prepare'",
+    "prepare-root": "cd ../../ && yarn && cd ./app/react",


We use yarn workspaces, so you can run yarn install in any package directory, and it will bootstrap everything. The cd part is therefore redundant

I also wonder why this script is needed at all. You shouldn't be able to run package scripts before installing everything anyway

Oh that's curious. It looks like Yarn just released 1.2.1, perhaps this was a bug fixed with workspaces.

Would you like me to remove the prepare-root script from this and retry from a clean git pull?

Yes please, but you can actually do yarn bootstrap --reset instead of clean pull

Hey, I commented further below, could you check in on this?

ndelangen · 2017-10-23T14:32:51Z

@AaronFriel Anything we need to do on our side to get this merged?

Who in the @storybooks/team works with windows and can do a review on this?
//cc @usulpro 🙇

AaronFriel · 2017-10-24T18:43:32Z

Hi, I think I understand what you meant now by the bootstrap script, which I had not run before attempting to work with a branch of the project. I just added a commit that makes "yarn bootstrap" run on Windows.

That said, I am a bit confused about the prepare-root part. If I just want to work on say, the @storybook/react package, I cannot simply git clone, work in the app/react folder, and use that package using a tool (e.g.: yalc). Why? yarn prepare won't run in app/react unless the workspace's dependencies have also been installed. This is what happens when I run yarn without the prepare-root script:

PS P:\c\gh\storybook\app\react> yarn
yarn install v1.2.1
[1/5] Validating package.json...
[2/5] Resolving packages...
[3/5] Fetching packages...
info fsevents@1.1.2: The platform "win32" is incompatible with this module.
info "fsevents@1.1.2" is an optional dependency and failed compatibility check. Excluding it from installation.
[4/5] Linking dependencies...
warning "eslint-config-airbnb@15.1.0" has incorrect peer dependency "eslint-plugin-jsx-a11y@^5.1.1".
warning "babel-jest@21.2.0" has unmet peer dependency "babel-core@^6.0.0 || ^7.0.0-alpha || ^7.0.0-beta || ^7.0.0".
warning "@storybook/addon-storyshots@3.2.12" has unmet peer dependency "babel-core@^6.26.0".
warning "@storybook/react-native@3.2.12" has unmet peer dependency "react-native@>=0.27.0".
warning "react-render-html@0.5.2" has incorrect peer dependency "react@^15.1.0".
warning "react-textarea-autosize@4.3.2" has incorrect peer dependency "react@>=0.14.0 <16.0.0".
warning "graphiql@0.11.5" has unmet peer dependency "prop-types@>=15.5.0".
warning "graphiql@0.11.5" has incorrect peer dependency "react@^15.6.0".
warning "graphiql@0.11.5" has incorrect peer dependency "react-dom@^15.6.0".
warning "react-addons-test-utils@15.6.2" has incorrect peer dependency "react-dom@^15.4.2".
warning "babel-loader@7.1.2" has unmet peer dependency "babel-core@6 || 7 || ^7.0.0-alpha || ^7.0.0-beta || ^7.0.0-rc".
warning "react-native@0.43.4" has incorrect peer dependency "react@16.0.0-alpha.6".
warning "babel-preset-react-app@3.0.3" has unmet peer dependency "babel-runtime@^6.23.0".
warning "react-modal@2.4.1" has incorrect peer dependency "react@^0.14.0 || ^15.0.0".
warning "react-modal@2.4.1" has incorrect peer dependency "react-dom@^0.14.0 || ^15.0.0".
warning "vue-loader@12.2.2" has unmet peer dependency "css-loader@*".
warning "@storybook/react-fuzzy@0.4.0" has incorrect peer dependency "react@^0.14.7 || ^15.0.0".
warning "react-komposer@2.0.0" has incorrect peer dependency "react@^0.14.7 || ^15.0.0".
warning "react-treebeard@2.0.3" has incorrect peer dependency "react@^15.5.4".
warning "react-treebeard@2.0.3" has incorrect peer dependency "react-dom@^15.5.4".
warning "webpack-dev-server@2.9.1" has unmet peer dependency "webpack@^2.2.0 || ^3.0.0".
warning "codemirror-graphql@0.6.11" has incorrect peer dependency "graphql@^0.6.0 || ^0.7.0 || ^0.8.0-b || ^0.8.0 || ^0.9.0 || ^0.10.0".
warning "react-komposer@1.13.1" has incorrect peer dependency "react@^0.14.3 || ^15.0.0".
warning "react-simple-di@1.2.0" has incorrect peer dependency "react@^0.14.0 || ^15.0.0".
warning "react-icon-base@2.1.0" has unmet peer dependency "prop-types@*".
warning "react-stubber@1.0.0" has incorrect peer dependency "react@^0.14.7 || ^15.0.0".
warning "radium@0.19.4" has incorrect peer dependency "react@^15.3.0".
[5/5] Building fresh packages...
$ node ../../scripts/prepare.js
ERR! FAILED: @storybook/react@3.2.12
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

Hypnosphi · 2017-10-24T19:00:41Z

There’re no such thing as “just work on @storybook/react package”. Even if it’s the only package where you make changes, you still need to run example app (located in examples/cra-kitchen-sink), eslint, and the tests. All of it requires bootstrapping (that is, installing the external dependencies, and cross-linking the internal ones)

AaronFriel · 2017-10-24T19:11:04Z

Oh, I actually had significant success using the tool yalc to just work on one package, while working with another repository (react-native-web). Yalc works by symlinking in every file and folder from a source directory (e.g.: storybook/react/app) to the appropriate package directory. It allowed me to make changes to the react package while not having to work with the rest of the packages.

AaronFriel · 2017-10-24T19:19:59Z

If it's a blocking issue for merging this, I can remove the prepare-root script. That said, it sounds like in your normal development workflow running yarn on the root directory is a no-op. It hurts nothing, and ensures yarn works in those directories.

Hypnosphi · 2017-11-06T04:32:09Z

My point was that as though you CAN work with only one package being set up, you SHOULDN'T do that, because you have to test how this interacts with current state of other packages anyway.

So yes, please remove the prepare-root script

Hypnosphi · 2017-11-06T04:41:22Z

By the way, maybe there's a sense in using cross-env in all the packages? We already have it in one: https://github.com/storybooks/storybook/blob/fad3ea3c907093ab62c56c7a7f6b9bb1e3367630/examples/vue-kitchen-sink/package.json

If you feel like that, you could add cross-env as a root dependency and prepend it to commands where needed. Feel free to contact us on slack if you have any questions about what some package does: https://now-examples-slackin-nqnzoygycp.now.sh/

codecov-io · 2017-11-06T20:38:45Z

Codecov Report

Merging #2051 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #2051   +/-   ##
======================================
  Coverage    22.2%   22.2%           
======================================
  Files         268     268           
  Lines        5872    5872           
  Branches      717     708    -9     
======================================
  Hits         1304    1304           
- Misses       4025    4035   +10     
+ Partials      543     533   -10

Impacted Files	Coverage Δ
lib/ui/src/libs/key_events.js	`23.25% <0%> (ø)`	⬆️
app/react-native/src/preview/story_kind.js	`0% <0%> (ø)`	⬆️
app/react/src/client/preview/error_display.js	`0% <0%> (ø)`	⬆️
...res__/update-addon-info/update-addon-info.input.js	`0% <0%> (ø)`	⬆️
...rc/modules/ui/components/left_panel/text_filter.js	`30.98% <0%> (ø)`	⬆️
lib/ui/src/modules/ui/routes.js	`0% <0%> (ø)`	⬆️
lib/ui/src/modules/api/actions/api.js	`50.47% <0%> (ø)`	⬆️
lib/ui/src/modules/ui/configs/handle_keyevents.js	`33.33% <0%> (ø)`	⬆️
lib/ui/src/modules/ui/containers/down_panel.js	`23.52% <0%> (ø)`	⬆️
lib/ui/src/modules/shortcuts/actions/shortcuts.js	`6.25% <0%> (ø)`	⬆️
... and 18 more

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 7aed50e...c9f5988. Read the comment docs.

AaronFriel · 2017-11-06T20:39:09Z

I changed other uses of the pattern [\w\d_]+= in scripts, e.g. in addons/comments/package.json.

I added cross-env to the root project, and removed prepare-root scripts. I also made sure that the vue-kitchen-sink, which references cross-env in its scripts has it as a dev dependency.

Hypnosphi · 2017-11-06T20:46:13Z

examples/vue-kitchen-sink/package.json

@@ -14,7 +14,7 @@
    "babel-loader": "^7.1.2",
    "babel-preset-env": "^1.6.0",
    "babel-preset-vue": "^1.2.1",
-    "cross-env": "^3.0.0",
+    "cross-env": "^5.1.1",


Is it still needed here, given that we have it in root now?

I wasn't sure. The kitchen sink is something people should be able to play with in isolation, right?

I assumed that because babel-core and babel-preset-env were also duplicated, that the package vue-kitchen-sink should be self-contained.

The kitchen sink is something people should be able to play with in isolation, right?

Not really. In our case, it‘s mainly used to test the changes made to other packages

Hypnosphi · 2017-11-06T22:01:21Z

Please resolve conflict in package.json and then run yarn in root to update yarn.lock file (the conflicts in yarn.lock will be resolved automatically)

AaronFriel · 2017-11-06T22:21:50Z

I rebased on master. Edit: And I got confused by GitHub's UI.

AaronFriel · 2017-11-06T22:55:30Z

Gosh that was fun. I think with that last commit all of the CI checks will be happy.

Hypnosphi · 2017-11-06T23:04:23Z

Great, thanks @AaronFriel !

ndelangen · 2017-11-06T23:18:03Z

Thank you @AaronFriel !

AaronFriel force-pushed the winqol branch from 6488bd1 to bae95e3 Compare October 14, 2017 18:01

ndelangen requested a review from usulpro October 14, 2017 21:40

ndelangen added the maintenance User-facing maintenance tasks label Oct 14, 2017

Hypnosphi requested changes Oct 16, 2017

View reviewed changes

Hypnosphi reviewed Nov 6, 2017

View reviewed changes

Hypnosphi mentioned this pull request Nov 6, 2017

cross-env versions available: 5.0.0, 4.0.0 #2172

Closed

Hypnosphi approved these changes Nov 6, 2017

View reviewed changes

AaronFriel added 3 commits November 6, 2017 16:20

Build scripts working on Windows.

8d03cda

Run bootstrap script with node.

5715039

Add cross-env as root devdep, remove unnecessary script.

ccd20f1

AaronFriel force-pushed the winqol branch from 448fbcb to ccd20f1 Compare November 6, 2017 22:21

AaronFriel added 4 commits November 6, 2017 16:23

Run test script with node.

db5288c

Merge branch 'master' into winqol

c952617

Fix package.json in app/vue

b893e3b

Update yarn.lock.

c9f5988

Hypnosphi merged commit c06a289 into storybookjs:master Nov 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes to build scripts for Windows. #2051

Fixes to build scripts for Windows. #2051

AaronFriel commented Oct 14, 2017

codecov bot commented Oct 14, 2017 •

edited

Loading

ndelangen commented Oct 14, 2017

Hypnosphi Oct 16, 2017 •

edited

Loading

AaronFriel Oct 16, 2017

AaronFriel Oct 16, 2017

Hypnosphi Oct 16, 2017

AaronFriel Nov 6, 2017

ndelangen commented Oct 23, 2017

AaronFriel commented Oct 24, 2017

Hypnosphi commented Oct 24, 2017

AaronFriel commented Oct 24, 2017

AaronFriel commented Oct 24, 2017

Hypnosphi commented Nov 6, 2017

Hypnosphi commented Nov 6, 2017

codecov-io commented Nov 6, 2017 •

edited by codecov bot

Loading

AaronFriel commented Nov 6, 2017

Hypnosphi Nov 6, 2017

AaronFriel Nov 6, 2017 •

edited

Loading

Hypnosphi Nov 6, 2017

Hypnosphi commented Nov 6, 2017 •

edited

Loading

AaronFriel commented Nov 6, 2017 •

edited

Loading

AaronFriel commented Nov 6, 2017

Hypnosphi commented Nov 6, 2017

ndelangen commented Nov 6, 2017

Fixes to build scripts for Windows. #2051

Fixes to build scripts for Windows. #2051

Conversation

AaronFriel commented Oct 14, 2017

What I did

codecov bot commented Oct 14, 2017 • edited Loading

Codecov Report

ndelangen commented Oct 14, 2017

Hypnosphi Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

AaronFriel Oct 16, 2017

Choose a reason for hiding this comment

AaronFriel Oct 16, 2017

Choose a reason for hiding this comment

Hypnosphi Oct 16, 2017

Choose a reason for hiding this comment

AaronFriel Nov 6, 2017

Choose a reason for hiding this comment

ndelangen commented Oct 23, 2017

AaronFriel commented Oct 24, 2017

Hypnosphi commented Oct 24, 2017

AaronFriel commented Oct 24, 2017

AaronFriel commented Oct 24, 2017

Hypnosphi commented Nov 6, 2017

Hypnosphi commented Nov 6, 2017

codecov-io commented Nov 6, 2017 • edited by codecov bot Loading

Codecov Report

AaronFriel commented Nov 6, 2017

Hypnosphi Nov 6, 2017

Choose a reason for hiding this comment

AaronFriel Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

Hypnosphi Nov 6, 2017

Choose a reason for hiding this comment

Hypnosphi commented Nov 6, 2017 • edited Loading

AaronFriel commented Nov 6, 2017 • edited Loading

AaronFriel commented Nov 6, 2017

Hypnosphi commented Nov 6, 2017

ndelangen commented Nov 6, 2017

codecov bot commented Oct 14, 2017 •

edited

Loading

Hypnosphi Oct 16, 2017 •

edited

Loading

codecov-io commented Nov 6, 2017 •

edited by codecov bot

Loading

AaronFriel Nov 6, 2017 •

edited

Loading

Hypnosphi commented Nov 6, 2017 •

edited

Loading

AaronFriel commented Nov 6, 2017 •

edited

Loading