Fix(react-native) add missing `ws` dependency #1174

joeybaker · 2017-05-31T21:38:23Z

Issue

ws was a dependency, but wasn't specified in package.json. We got away with this b/c ws is a dependency of react-native, but that causes a problem because the version of ws used by storybook is non-deterministic. This can cause the following error on startup:

this.wsServer = _ws2.default.Server({ server: this.httpServer });
                                 ^

TypeError: Class constructor WebSocketServer cannot be invoked without 'new'
    at new Server (.../node_modules/@storybook/react-native/dist/server/index.js:48:34)

What I did

Added new, and added ws to package.json

How to test

Just run storybook start. It now works. Before, it would crash.

Fixes this error on startup: ``` this.wsServer = _ws2.default.Server({ server: this.httpServer }); ^ TypeError: Class constructor WebSocketServer cannot be invoked without 'new' at new Server (.../node_modules/@storybook/react-native/dist/server/index.js:48:34) ```

codecov · 2017-05-31T21:49:33Z

Codecov Report

Merging #1174 into master will not change coverage.
The diff coverage is 0%.

@@           Coverage Diff            @@
##           master    #1174    +/-   ##
========================================
  Coverage   13.31%   13.31%            
========================================
  Files         199      199            
  Lines        4588     4588            
  Branches      724      522   -202     
========================================
  Hits          611      611            
- Misses       3349     3515   +166     
+ Partials      628      462   -166

Impacted Files	Coverage Δ
app/react-native/src/server/index.js	`0% <0%> (ø)`	⬆️
app/react-native/src/bin/storybook.js	`0% <0%> (ø)`	⬆️
app/react-native/src/preview/story_kind.js	`0% <0%> (ø)`	⬆️
lib/ui/src/modules/ui/containers/left_panel.js	`20% <0%> (ø)`	⬆️
addons/info/src/components/markdown/code.js	`0% <0%> (ø)`	⬆️
addons/storyshots/src/test-bodies.js	`0% <0%> (ø)`	⬆️
addons/knobs/src/components/types/Select.js	`7.14% <0%> (ø)`	⬆️
app/react/src/client/preview/client_api.js	`39.28% <0%> (ø)`	⬆️
addons/info/src/components/PropVal.js	`0% <0%> (ø)`	⬆️
addons/knobs/src/components/PropField.js	`10.41% <0%> (ø)`	⬆️
... and 33 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 7b6c00c...9a139ec. Read the comment docs.

ndelangen · 2017-05-31T22:19:36Z

Thanks, I'll ask another react-native user to give this a test. on positive I'll merge it!

Can you explain to error message / bug a bit more? So it can be tested better and have a good changelog later on.

joeybaker · 2017-05-31T22:21:55Z

Sure? I just ran storybook start and got the stated error. It looks like an oversight/typo/node version issue.

Didn't test

joeybaker · 2017-05-31T23:48:19Z

Howdy @shilman – how would you like me to test this? There don't appear to be any tests setup for this kind of thing as far as I can tell?

shilman · 2017-05-31T23:51:38Z

Hi @joeybaker FYI I dismissed @ndelangen 's review because he didn't test it, not because you didn't provide tests.

We were discussing this in Slack and are a little confused about the change, since we've all been using storybook/react-native without problems. I'll work with you to get this in, or at least figure out what's going on.

joeybaker · 2017-05-31T23:55:43Z

hmmm… I'm not quite sure how I'm special then :). I'm happy to hope into a slack/IRC if you want.

FWIW, the ws docs pretty clearly indicate that they require new to be used.

shilman · 2017-06-01T01:12:28Z

@joeybaker Sure, jump into slack, or we can discuss here. Either way!

https://storybooks-slackin.herokuapp.com/

If you can provide a broken repro, I'd love to look at it. I'll also provide you a working repro, and you can try it on your machine.

sgtsquiggs · 2017-06-01T16:50:45Z

This fix works for me locally as well.

shilman · 2017-06-01T17:26:17Z

@joeybaker don't doubt your fix, just trying to understand why it's working for most people. here's the simple repro that works on my machine:

shell1:

yarn create react-native-app rnapp
cd rnapp
getstorybook
yarn storybook

shell2:

open http://localhost:7007
yarn run ios

If I understand your fix properly this should break, but it doesn't. Any ideas?

joeybaker · 2017-06-01T17:31:34Z

Hmm… a few thoughts:

I'm running node v7.9, maybe there's a difference there?
I didn't use CRNA, maybe the dependency order matters?

shilman · 2017-06-01T17:36:20Z

@joeybaker mind sharing an analogous repro? I'd love to figure this out

atticoos · 2017-06-01T17:36:32Z

This is a little bit of a tricky one, the new keyword was introduced in v2.0.0.

If you notice, @storybook/react-native does not specify the version of ws, it's implicitly using React Native's dependency on ws, which is at v1.1.x.

⚠️ ⚠️ Making this change alone will break for users who do not specify their own ws@>=2.0.0 dependency

@joeybaker - does your project define it's own ws dependency above 1.x?

Next steps

We should add ws as a dependency to @storyboard/react-native specifying 3.x

Could someone double check my thinking here: is it okay if you specify a dependency at a version that is different from another dependency who has the same module as their dependency? (that's a mouthful)

For example:

your-project
 - [email protected]
    - [email protected]
 - [email protected]
     ^-------- does this conflict with react-native's [email protected]?

atticoos · 2017-06-01T17:53:18Z

I think this is okay? It's marked as "extraneous", but I think it would still use it?

Package A (test-a)

[email protected]

Package B (test-b)

package-a
[email protected]

yields

[email protected] /Users/atticus/Desktop/test-b
├─┬ [email protected] invalid
│ └── [email protected] extraneous
└─┬ [email protected]
  ├── [email protected]
  └── [email protected]

joeybaker · 2017-06-01T17:53:50Z

@ajwhite Good call! We specify v1.1.1 for ws, but yarn.lock has:

[email protected]:
  version "3.0.0"
  resolved "https://registry.yarnpkg.com/ws/-/ws-3.0.0.tgz#98ddb00056c8390cb751e7788788497f99103b6c"
  dependencies:
    safe-buffer "~5.0.1"
    ultron "~1.1.0"

ws@^1.1.0:
  version "1.1.4"
  resolved "https://registry.yarnpkg.com/ws/-/ws-1.1.4.tgz#57f40d036832e5f5055662a397c4de76ed66bf61"
  dependencies:
    options ">=0.0.5"
    ultron "1.0.x"

ws@^2.0.3:
  version "2.3.1"
  resolved "https://registry.yarnpkg.com/ws/-/ws-2.3.1.tgz#6b94b3e447cb6a363f785eaf94af6359e8e81c80"
  dependencies:
    safe-buffer "~5.0.1"
    ultron "~1.1.0"

Resulting in:

cat node_modules/ws/package.json | json version
→ 3.0.0

So, if storybook/react-native doesn't specify a ws version it'll get v3 in our project.

The answer to your second question is that the two ws dependencies don't conflict – that's part of the genius of npm's original design :)

I completely agree that storybook/react-native should always specify all it's dependencies.

atticoos · 2017-06-01T17:55:54Z

The answer to your second question is that the two ws dependencies don't conflict – that's part of the genius of npm's original design :)

😃 thanks for clearing this up! I felt like this was the answer, but wanted to be 100%.

So it sounds like we can add the appropriate version as a dependency and we should be good here. I think we can all agree being on the later version is the proper path forward? Otherwise we can specify 1.1.0 as the dependency version and keep things w/o the new keyword. But I'm not against moving to 3.x

atticoos

Since the current form will lead to a breaking change, dropping in this review to make sure we add the ws dependency to @storybook/react-native before merging

It's a dependency already!

joeybaker · 2017-06-01T20:17:01Z

@ajwhite I added ws to package.json, but I don't have time to test this out. Please feel free to edit this PR if need-be.

atticoos · 2017-06-01T20:30:23Z

Thanks @joeybaker, if anyone can give this a test that would be great. Otherwise I can try to follow up tomorrow 📆

dependency added, haven't been able to test yet

shilman

@joeybaker @ajwhite tested on my local machine with CRNA. Before this PR:

[email protected] /Users/shilman/projects/storybook/new/rnapp
├── UNMET PEER DEPENDENCY [email protected]
└─┬ UNMET PEER DEPENDENCY [email protected]
  ├─┬ [email protected]
  │ └── [email protected] 
  └── [email protected]

After this PR:

[email protected] /Users/shilman/projects/storybook/new/rnapp
├─┬ @storybook/[email protected] invalid
│ ├── UNMET PEER DEPENDENCY [email protected] extraneous
│ └── [email protected]  extraneous
├── UNMET PEER DEPENDENCY [email protected]
└─┬ UNMET PEER DEPENDENCY [email protected]
  ├─┬ [email protected]
  │ └── [email protected] 
  └── [email protected]

And it's working for me. Merging. Thanks for the fix @joeybaker and @ajwhite !

ndelangen previously approved these changes May 31, 2017

View reviewed changes

ndelangen added bug react-native labels May 31, 2017

atticoos previously requested changes Jun 1, 2017

View reviewed changes

Chore(deps) install ws

57abc9d

It's a dependency already!

joeybaker changed the title ~~Fix(react-native) add missing new~~ Fix(react-native) add missing ws dependency Jun 1, 2017

Merge branch 'master' into patch-1

9a139ec

shilman approved these changes Jun 5, 2017

View reviewed changes

shilman merged commit d59feed into storybookjs:master Jun 5, 2017

joeybaker deleted the patch-1 branch June 5, 2017 03:31

snyk-bot mentioned this pull request Apr 14, 2021

[Snyk] Fix for 1 vulnerabilities mcaj-admin/storybook#86

Open

This was referenced Feb 1, 2022

[Snyk] Fix for 1 vulnerabilities Watemlifts/storybook#286

Merged

[Snyk] Security upgrade @mdx-js/mdx from 1.6.22 to 2.0.0 Watemlifts/storybook#287

Closed

This was referenced Jul 4, 2023

[Snyk] Fix for 1 vulnerabilities lili2311/storybook#140

Open

[Snyk] Security upgrade @mdx-js/mdx from 1.6.22 to 2.0.0 lili2311/storybook#141

Open

This was referenced Aug 12, 2023

[Snyk-dev] Security upgrade @mdx-js/mdx from 1.6.22 to 2.0.0 lili2311/storybook#146

Open

[Snyk-dev] Fix for 4 vulnerabilities lili2311/storybook#147

Open

lili2311 mentioned this pull request Sep 22, 2023

[Snyk-dev] Fix for 2 vulnerabilities lili2311/storybook#164

Open

lili2311 mentioned this pull request Oct 1, 2023

[Snyk] Fix for 1 vulnerabilities lili2311/storybook#180

Open

Bhanditz mentioned this pull request Oct 1, 2023

[Snyk] Fix for 1 vulnerabilities Bhanditz/storybook#117

Open

adrukh mentioned this pull request Oct 1, 2023

[Snyk] Fix for 1 vulnerabilities adrukh/storybook#373

Open

adrianblynch mentioned this pull request Oct 1, 2023

[Snyk] Fix for 1 vulnerabilities adrianblynch/storybook#231

Open

Bhanditz mentioned this pull request Oct 1, 2023

[Snyk] Fix for 1 vulnerabilities Bhanditz/storybook#118

Open

This was referenced Oct 1, 2023

[Snyk] Fix for 1 vulnerabilities adrianblynch/storybook#232

Open

[Snyk] Fix for 1 vulnerabilities adrianblynch/storybook#234

Open

This was referenced Nov 25, 2023

[Snyk-dev] Fix for 9 vulnerabilities lili2311/storybook#205

Open

[Snyk-dev] Fix for 3 vulnerabilities lili2311/storybook#206

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix(react-native) add missing `ws` dependency #1174

Fix(react-native) add missing `ws` dependency #1174

joeybaker commented May 31, 2017 •

edited

Loading

codecov bot commented May 31, 2017 •

edited

Loading

ndelangen commented May 31, 2017

joeybaker commented May 31, 2017

joeybaker commented May 31, 2017

shilman commented May 31, 2017

joeybaker commented May 31, 2017 •

edited

Loading

shilman commented Jun 1, 2017 •

edited

Loading

sgtsquiggs commented Jun 1, 2017

shilman commented Jun 1, 2017

joeybaker commented Jun 1, 2017

shilman commented Jun 1, 2017

atticoos commented Jun 1, 2017 •

edited

Loading

atticoos commented Jun 1, 2017

joeybaker commented Jun 1, 2017

atticoos commented Jun 1, 2017 •

edited

Loading

atticoos left a comment

joeybaker commented Jun 1, 2017 •

edited

Loading

atticoos commented Jun 1, 2017

shilman left a comment

Fix(react-native) add missing ws dependency #1174

Fix(react-native) add missing ws dependency #1174

Conversation

joeybaker commented May 31, 2017 • edited Loading

Issue

What I did

How to test

codecov bot commented May 31, 2017 • edited Loading

Codecov Report

ndelangen commented May 31, 2017

joeybaker commented May 31, 2017

joeybaker commented May 31, 2017

shilman commented May 31, 2017

joeybaker commented May 31, 2017 • edited Loading

shilman commented Jun 1, 2017 • edited Loading

sgtsquiggs commented Jun 1, 2017

shilman commented Jun 1, 2017

joeybaker commented Jun 1, 2017

shilman commented Jun 1, 2017

atticoos commented Jun 1, 2017 • edited Loading

Next steps

atticoos commented Jun 1, 2017

joeybaker commented Jun 1, 2017

atticoos commented Jun 1, 2017 • edited Loading

atticoos left a comment

Choose a reason for hiding this comment

joeybaker commented Jun 1, 2017 • edited Loading

atticoos commented Jun 1, 2017

shilman left a comment

Choose a reason for hiding this comment

Fix(react-native) add missing `ws` dependency #1174

Fix(react-native) add missing `ws` dependency #1174

joeybaker commented May 31, 2017 •

edited

Loading

codecov bot commented May 31, 2017 •

edited

Loading

joeybaker commented May 31, 2017 •

edited

Loading

shilman commented Jun 1, 2017 •

edited

Loading

atticoos commented Jun 1, 2017 •

edited

Loading

atticoos commented Jun 1, 2017 •

edited

Loading

joeybaker commented Jun 1, 2017 •

edited

Loading