Fixed date/color knobs by forcing a single version of react #1345

ndelangen · 2017-06-22T23:28:12Z

Issue: broken UI widgets caused by multiple instances of react during development

What I did

Add a webpack resolve for react so it's always loaded from a single place

How to test

bootstrap
run storybook kitchensink
open button knobs story
use color and datetime picker

codecov · 2017-06-22T23:29:05Z

Codecov Report

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

@@           Coverage Diff           @@
##           master    #1345   +/-   ##
=======================================
  Coverage   13.76%   13.76%           
=======================================
  Files         202      202           
  Lines        4621     4621           
  Branches      598      618   +20     
=======================================
  Hits          636      636           
+ Misses       3473     3462   -11     
- Partials      512      523   +11

Impacted Files	Coverage Δ
app/react/src/server/config/webpack.config.prod.js	`0% <ø> (ø)`	⬆️
app/react/src/server/config/webpack.config.js	`0% <ø> (ø)`	⬆️
app/react-native/src/bin/storybook-build.js	`0% <0%> (ø)`	⬆️
lib/ui/src/modules/api/configs/init_api.js	`40.47% <0%> (ø)`	⬆️
addons/info/src/components/markdown/htags.js	`0% <0%> (ø)`	⬆️
app/react/src/demo/Welcome.js	`0% <0%> (ø)`	⬆️
...src/server/config/WatchMissingNodeModulesPlugin.js	`0% <0%> (ø)`	⬆️
.../ui/src/modules/ui/components/left_panel/header.js	`27.58% <0%> (ø)`	⬆️
addons/info/src/components/Node.js	`0% <0%> (ø)`	⬆️
addons/storyshots/src/require_context.js	`0% <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 bf6faea...a81c2d4. Read the comment docs.

shilman · 2017-06-22T23:29:57Z

examples/cra-kitchen-sink/src/stories/index.js

@@ -58,6 +58,7 @@ storiesOf('Button', module)

    // NOTE: color picker is currently broken


@ndelangen if it's fixed, please remove this 😄

shilman · 2017-06-22T23:30:57Z

lib/cli/package.json

@@ -20,9 +20,6 @@
    "type": "git",
    "url": "https://github.com/storybooks/storybook.git"
  },
-  "scripts": {


@ndelangen remove opencollective deps?

Yeah the postinstall was messing with hoisting which I experimented with, When refactoring the cli I will add a opencollective notice during first run or something like it.

shilman · 2017-06-22T23:32:19Z

scripts/prepublish.js

@@ -9,11 +9,15 @@ shell.echo(chalk.bold(`${packageJson.name}@${packageJson.version}`));
 shell.echo(chalk.gray('\n=> Clean dist.'));
 shell.rm('-rf', 'dist');

+const pwd = shell.pwd();
+shell.echo(`pwd = ${pwd}`);
+shell.cd(path.join(__dirname, '..'));


@ndelangen what's this for?

Babel WAS run inside each folder, using the babel binary from root, but presets from package folders.

This changes so the directory where babel is run from IS root, so presets are taken from there. This will allow us to clean up the babel presets dependencies in packages a bit.

Can you explain this a bit more? Doesn't babel end up looking for the preset in root if it doesn't find it in the package anyway (w/ standard node_modules resolution algorithm).

storybookbot · 2017-06-22T23:37:46Z

	Fails
🚫	PR is not labeled with one of: ["cleanup","breaking","feature","bug","documentation","maintenance","greenkeeper","other"]

Generated by 🚫 dangerJS

shilman

@ndelangen this is beautiful. i don't understand what's going on, so some comments would help, and see my other comments as well, but this is a great fix. thanks for figuring it out! WIZARD.

tmeasday · 2017-06-23T01:16:19Z

Wouldn't it be better to sort out the actual problem? Why are there multiple instances of React?

tmeasday

I think this PR should be split up into different parts, it seems to be doing a few things at once that should be discussed separately

tmeasday · 2017-06-23T01:18:16Z

.gitignore

@@ -1,17 +1,20 @@
 node_modules
 *.log
 .idea
+
+package-lock.json


I don't think we should be ignoring these files (also yarn.lock)

@tmeasday see my PR here: #1338

We tried merging in the package lock, but it broke everything, and I pulled it out. I think until somebody figures out what to do here, we are better off not checking them in and ignoring them so that they don't accidentally get checked in.

I guess that makes sense but it certainly feels weird to be ignoring a file that npm tells you to check in. We should open an issue about this I guess

tmeasday · 2017-06-23T01:20:25Z

scripts/prepublish.js

@@ -9,11 +9,15 @@ shell.echo(chalk.bold(`${packageJson.name}@${packageJson.version}`));
 shell.echo(chalk.gray('\n=> Clean dist.'));
 shell.rm('-rf', 'dist');

+const pwd = shell.pwd();
+shell.echo(`pwd = ${pwd}`);
+shell.cd(path.join(__dirname, '..'));


Can you explain this a bit more? Doesn't babel end up looking for the preset in root if it doesn't find it in the package anyway (w/ standard node_modules resolution algorithm).

tmeasday · 2017-06-23T02:56:35Z

If my understanding is correct, this issue is caused by multiple loading problems due to npm linking.

Is it an issue that affects actual apps (i.e. w/o linking)? If not, we should NOT be changing the behaviour of the library to serve our testing purposes IMO.

There are two ways forward w/o this change:

Use test-cra which thanks to Setup test-cra to use tarballs #1331 doesn't use linking and so should be immune to this kind of issue.
Think about ways to solve the multiple loading problem. Can lerna help us here? For instance, can we hoist the react dependency up to the top-level so all packages end up using that?

shilman · 2017-07-15T20:20:05Z

@ndelangen Date/color knobs seem to be working again, independent of this change. OK if I close this?

shilman · 2017-07-22T06:40:57Z

@ndelangen can we close this now?

ndelangen added 5 commits June 23, 2017 00:04

WIP

359e74e

ADD webpack alias to prevent duplicate react

078b6af

CHANGE prepublish script to run from root (using presets from root)

28a1157

CLEANUP gitignore

c655827

ADD webpack resolve to prod version

a81c2d4

ndelangen requested a review from shilman June 22, 2017 23:28

ndelangen self-assigned this Jun 22, 2017

ndelangen added the in progress label Jun 22, 2017

shilman reviewed Jun 22, 2017

View reviewed changes

shilman requested changes Jun 22, 2017

View reviewed changes

shilman added the bug label Jun 22, 2017

shilman changed the title ~~Unbreak npm~~ Fixed broken date and color knobs by forcing a single version of react Jun 22, 2017

shilman changed the title ~~Fixed broken date and color knobs by forcing a single version of react~~ Fixed date and color knobs by forcing a single version of react Jun 22, 2017

shilman changed the title ~~Fixed date and color knobs by forcing a single version of react~~ Fixed date/color knobs by forcing a single version of react Jun 22, 2017

tmeasday requested changes Jun 23, 2017

View reviewed changes

tmeasday mentioned this pull request Jun 23, 2017

Knob DateType breaks storybook ui #1318

Closed

ndelangen closed this Jul 26, 2017

ndelangen deleted the unbreak-npm branch July 26, 2017 05:13

This was referenced Sep 19, 2018

Update graphql in addons/graphql/package.json from ^0.13.2 to 14.0.2 Hypnosphi/storybook#5

Open

Update graphql in examples/official-storybook/package.json from ^0.13.2 to 14.0.2 Hypnosphi/storybook#6

Open

dependabot-preview bot mentioned this pull request May 28, 2019

Bump eslint-plugin-import from 2.17.2 to 2.17.3 #6894

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed date/color knobs by forcing a single version of react #1345

Fixed date/color knobs by forcing a single version of react #1345

ndelangen commented Jun 22, 2017

codecov bot commented Jun 22, 2017 •

edited

Loading

shilman Jun 22, 2017 •

edited

Loading

shilman Jun 22, 2017

ndelangen Jun 22, 2017

shilman Jun 22, 2017

ndelangen Jun 22, 2017

tmeasday Jun 23, 2017

storybookbot commented Jun 22, 2017

shilman left a comment

tmeasday commented Jun 23, 2017

tmeasday left a comment

tmeasday Jun 23, 2017

shilman Jun 23, 2017

tmeasday Jun 23, 2017

tmeasday Jun 23, 2017

tmeasday Jun 23, 2017

tmeasday commented Jun 23, 2017

shilman commented Jul 15, 2017

shilman commented Jul 22, 2017

		@@ -58,6 +58,7 @@ storiesOf('Button', module)

		// NOTE: color picker is currently broken

Fixed date/color knobs by forcing a single version of react #1345

Fixed date/color knobs by forcing a single version of react #1345

Conversation

ndelangen commented Jun 22, 2017

What I did

How to test

codecov bot commented Jun 22, 2017 • edited Loading

Codecov Report

shilman Jun 22, 2017 • edited Loading

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

storybookbot commented Jun 22, 2017

shilman left a comment

Choose a reason for hiding this comment

tmeasday commented Jun 23, 2017

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

Choose a reason for hiding this comment

tmeasday commented Jun 23, 2017

shilman commented Jul 15, 2017

shilman commented Jul 22, 2017

codecov bot commented Jun 22, 2017 •

edited

Loading

shilman Jun 22, 2017 •

edited

Loading