SWITCH to circleci over travisCI && CHANGE lerna bootstrap procedure: #1486

ndelangen · 2017-07-19T06:11:23Z

Issue:

TravisCI is taking a long time to build
Fixes: Parallelise Travis build #1127
Bootstrapping takes a long time
A lot of modules are duplicated during bootstrap
Because of this modules are not singletons during development.
This leads to a list of problems, like duplicate react, no channel in lib/addons..
Storyshots testing is disabled in cra-kitchen-sink

What I did

Configure CircleCI (in parallel)
Remove TravisCI
Use yarn for installation of packages
Hoist all dependencies to root using --hoist
Hoist known packages to root using a custom script
Test storyshots in cra-kitchen-sink

How to test

This branch should be build with CircleCI
Doing a clean install and bootstrap on this branch should be successful
After bootstrap unit tests should pass
After bootstrap starting example/cra-kitchen-sink should be successful
You should feel a feeling of relief and inner happiness knowing how much faster the CI is.

- Use yarn for installation - Hoist all dependencies to root - Hoist known packages to root - Test storyshots in cra-kitchen-sink

tmeasday

Can we separate these? The circle script seems awesome, but it seems an orthogonal concern to "true hoisting"+testing cra-kitchen-sink

tmeasday · 2017-07-19T06:36:15Z

examples/cra-kitchen-sink/.storybook/config.js

@@ -20,3 +20,5 @@ function loadStories() {
 }

 configure(loadStories, module);
+
+export { getStorybook };


What's going on with this change?

getStorybook returns a hash-table of stories.

If the config would export this, we can make storyshots framework agnostic 🎉

That seems great, although can you explain a bit more why? Should this be part of this PR? It sounds like a seperate thing

how does exporting getStorybook help exactly? i'm already familiar with the function.

codecov · 2017-07-19T08:11:34Z

Codecov Report

Merging #1486 into master will increase coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1486      +/-   ##
==========================================
+ Coverage   14.54%   14.54%   +<.01%     
==========================================
  Files         202      202              
  Lines        4649     4647       -2     
  Branches      502      497       -5     
==========================================
  Hits          676      676              
  Misses       3549     3549              
+ Partials      424      422       -2

Impacted Files	Coverage Δ
lib/addons/src/index.js	`0% <0%> (ø)`	⬆️
addons/storyshots/src/index.js	`0% <0%> (ø)`	⬆️
app/react/src/server/babel_config.js	`44.82% <0%> (ø)`	⬆️
...codemod/src/transforms/update-organisation-name.js	`40.62% <0%> (ø)`	⬆️
addons/info/src/components/PropVal.js	`0% <0%> (ø)`	⬆️
lib/ui/src/modules/ui/routes.js	`0% <0%> (ø)`	⬆️
addons/knobs/src/components/types/Number.js	`8.06% <0%> (ø)`	⬆️
...src/server/config/WatchMissingNodeModulesPlugin.js	`0% <0%> (ø)`	⬆️
addons/info/src/components/markdown/htags.js	`0% <0%> (ø)`	⬆️
addons/knobs/src/components/PropForm.js	`8.51% <0%> (ø)`	⬆️
... and 19 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 860e952...3be8c8d. Read the comment docs.

ndelangen · 2017-07-19T08:23:02Z

No we cannot split this, TravisCI fails and I just don't see a good reason to spend time on fixing it.
https://travis-ci.org/storybooks/storybook/builds/255133024

What's your reason for splitting? is it too big to review? Is there any other way to get this approved? I'm interested in doing a demo video anyway, would that help?

tmeasday · 2017-07-19T08:49:09Z

Could you not first do the circle changes and then a second pr targeting the first with the cra-ks changes.

Yes, I'll admit I have a lot of trouble properly reviewing these big PRs. There are some important subtleties around this symlinking stuff that it is hard enough to keep straight in my head without also needing to figure out if any given change is relevant to the fix.

Small PRs=non-linearly better imo

shilman · 2017-07-19T08:59:59Z

addons/storyshots/src/index.js

-      filename: configPath,
-      dirname: configDirPath,
-    };
+    // const babelConfig = loadBabelConfig(configDirPath);


Well correct me if I'm wrong: jest can do babel transpilation, why are we doing it?
I don't understand why we're using require.requireActual either

if we're not using it, why comment it out? why not just delete it?

shilman · 2017-07-19T09:02:35Z

examples/react-native-vanilla/storybook/storybook.js

@@ -19,4 +19,5 @@ setTimeout(
 );

 AppRegistry.registerComponent('ReactNativeVanilla', () => StorybookUI);
-export default StorybookUI;
+
+export { StorybookUI as default, getStorybook };


same question here

shilman · 2017-07-19T09:05:26Z

package.json

+    "bootstrap:docs": "cd docs && yarn install",
+    "bootstrap:react-native-vanilla": "lerna exec --scope react-native-vanilla -- yarn install",
+    "bootstrap:test-cra": "npm run build-packs && lerna exec --scope test-cra -- yarn install",
+    "build": "npm run bootstrap:docs && npm run docs:build",


why do we need this? bootstrap:docs alone should be fine?

I don't think I'm calling this script anywhere, will remove.

i think you can get rid of docs:build too

shilman · 2017-07-19T09:06:26Z

package.json

@@ -25,15 +27,14 @@
    "lint:js": "eslint . --cache --cache-location=.cache/eslint --ext .js,.jsx,.json",
    "lint:md": "remark .",
    "publish": "lerna publish",
+    "start": "serve ./docs/out --single",


add serve as dev dependency? i think you are mixing in some of your docsv2 stuff in here?

will remove

shilman · 2017-07-19T09:06:59Z

scripts/hoist-internals.js

+  .then(() => {
+    shell.echo(chalk.green('COMPLETE'), chalk.gray('=> Hoisting internal packages'));
+  })
+  .catch(() => {


print error

shilman

commented but did not test yet

ndelangen · 2017-07-19T21:58:19Z

🎉

tmeasday · 2017-07-19T22:50:13Z

lib/addons/src/index.js

@@ -2,7 +2,7 @@ export class AddonStore {
  constructor() {
    this.loaders = {};
    this.panels = {};
-    this.channel = null;
+    this.channel = { on() {}, emit() {} };


Hmm, I think this change is going to just make it harder to debug problems with multiple loading. Why did you add it @ndelangen ?

Great point. Let's take this out unless it's necessary!

This was giving us so much grief, I thought we could do without?

To the best of my abilities, everything actually works, there's no multiple loading going on.

It should never occur to not have set the channel.

Having said that, I think it's just really wrong to have a setup where side effects have such a major impact on whether a test can be run.

The channels is completely unneeded to run the snapshots for example, yet they have caused a lot of pain for a lot of users.

I just want people to be able to use and develop this. I think this code facilitates that better then without.

@ndelangen completely disagree. if this doesn't get overwritten it will cause an error to fail silently! having been bit by this bug before, it will be even harder to debug when there is a problem!

I have to agree with @shilman here, it's better that our or even a user's tests fail than they can't actually run storybook or an addon in the first place and can't figure out why (it's hard enough with the channel undefined error in the first place).

If we want to have a way to "ignore" the error, maybe that's a feature worth adding, if it's sometimes impossible to avoid the multi-loading problem. Something like initStoryshots({ nullChannel: true})?

SWITCH to circleci over travisCI && CHANGE lerna bootstrap procedure:

299a53a

- Use yarn for installation - Hoist all dependencies to root - Hoist known packages to root - Test storyshots in cra-kitchen-sink

ndelangen added maintenance User-facing maintenance tasks needs review labels Jul 19, 2017

ndelangen self-assigned this Jul 19, 2017

ndelangen requested review from tmeasday, shilman and roonyh July 19, 2017 06:11

export getStorybook from config

cad2b8d

tmeasday requested changes Jul 19, 2017

View reviewed changes

ndelangen force-pushed the hoisting-and-circleci branch from 8abffd8 to ca112c0 Compare July 19, 2017 07:57

CI run jest in series && add dummy deploy

9faced5

ndelangen force-pushed the hoisting-and-circleci branch from ca112c0 to 9faced5 Compare July 19, 2017 08:11

shilman reviewed Jul 19, 2017

View reviewed changes

shilman requested changes Jul 19, 2017

View reviewed changes

ndelangen added 3 commits July 19, 2017 17:38

REVERT storyshots changes

9469f5d

CLEANUP junk npm scripts

f1ae387

CLEANUP && improve hoist internals script

3be8c8d

shilman approved these changes Jul 19, 2017

View reviewed changes

shilman merged commit 251c563 into master Jul 19, 2017

shilman deleted the hoisting-and-circleci branch July 19, 2017 21:53

tmeasday reviewed Jul 19, 2017

View reviewed changes

shilman mentioned this pull request Jul 19, 2017

Undo channel stubbing - causes NPM version errors to fail silently #1494

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SWITCH to circleci over travisCI && CHANGE lerna bootstrap procedure: #1486

SWITCH to circleci over travisCI && CHANGE lerna bootstrap procedure: #1486

ndelangen commented Jul 19, 2017

tmeasday left a comment

tmeasday Jul 19, 2017

ndelangen Jul 19, 2017

tmeasday Jul 19, 2017

shilman Jul 19, 2017

codecov bot commented Jul 19, 2017 •

edited

Loading

ndelangen commented Jul 19, 2017

tmeasday commented Jul 19, 2017

shilman Jul 19, 2017

ndelangen Jul 19, 2017

shilman Jul 19, 2017

shilman Jul 19, 2017

shilman Jul 19, 2017

ndelangen Jul 19, 2017

shilman Jul 19, 2017

shilman Jul 19, 2017

ndelangen Jul 19, 2017

shilman Jul 19, 2017

shilman left a comment

ndelangen commented Jul 19, 2017

tmeasday Jul 19, 2017

shilman Jul 19, 2017

ndelangen Jul 19, 2017

shilman Jul 19, 2017

tmeasday Jul 20, 2017

tmeasday Jul 20, 2017

SWITCH to circleci over travisCI && CHANGE lerna bootstrap procedure: #1486

SWITCH to circleci over travisCI && CHANGE lerna bootstrap procedure: #1486

Conversation

ndelangen commented Jul 19, 2017

What I did

How to test

tmeasday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 19, 2017 • edited Loading

Codecov Report

ndelangen commented Jul 19, 2017

tmeasday commented Jul 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shilman left a comment

Choose a reason for hiding this comment

ndelangen commented Jul 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 19, 2017 •

edited

Loading