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

websockets (ws) removed socket.upgradeReq, so use req instead #1337

Merged
merged 4 commits into from
Jun 23, 2017

Conversation

Gongreg
Copy link
Member

@Gongreg Gongreg commented Jun 22, 2017

Issue

We used not existing property, so it was impossible to use manual id in RN storybook.

What I did

Changed upgradeReq to req

How to test

https://github.com/wix/react-native-storybook-example .
Build the project and run npm run storybook-hosted.
Enter the code displayed in browser to the app. Without the change it won't work.

@Gongreg Gongreg added the bug label Jun 22, 2017
@Gongreg Gongreg requested a review from shilman June 22, 2017 12:51
const params = socket.upgradeReq && socket.upgradeReq.url
? querystring.parse(socket.upgradeReq.url.substr(1))
: {};
const params = req.url ? querystring.parse(req.url.substr(1)) : {};
Copy link
Member

Choose a reason for hiding this comment

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

@Gongreg do we need req && req.url like in the previous code?

Copy link
Member Author

Choose a reason for hiding this comment

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

Most likely no. Just safety check for req.url.substr().

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.

Approved, but build is breaking so I want to look into that before we merge. Thanks for your patience!

@shilman shilman mentioned this pull request Jun 22, 2017
2 tasks
@codecov
Copy link

codecov bot commented Jun 22, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1337      +/-   ##
==========================================
+ Coverage   13.75%   13.76%   +<.01%     
==========================================
  Files         202      202              
  Lines        4623     4621       -2     
  Branches      514      576      +62     
==========================================
  Hits          636      636              
+ Misses       3543     3492      -51     
- Partials      444      493      +49
Impacted Files Coverage Δ
app/react-native/src/server/index.js 0% <0%> (ø) ⬆️
app/react/src/client/preview/reducer.js 0% <0%> (ø) ⬆️
...rc/modules/ui/components/left_panel/text_filter.js 33.33% <0%> (ø) ⬆️
app/react/src/client/preview/init.js 0% <0%> (ø) ⬆️
addons/knobs/src/KnobStore.js 6.81% <0%> (ø) ⬆️
addons/info/src/components/Node.js 0% <0%> (ø) ⬆️
app/react/src/server/babel_config.js 44.82% <0%> (ø) ⬆️
addons/info/src/components/PropVal.js 0% <0%> (ø) ⬆️
addons/storyshots/src/require_context.js 0% <0%> (ø) ⬆️
app/react/src/client/preview/config_api.js 0% <0%> (ø) ⬆️
... and 28 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 a30166d...1264bec. Read the comment docs.

@shilman shilman merged commit bb1bd8b into master Jun 23, 2017
@shilman shilman changed the title websockets (ws) had removed socket.upgradeReq property, so instead we use req now websockets (ws) removed socket.upgradeReq property, so use req instead Jun 26, 2017
@shilman shilman changed the title websockets (ws) removed socket.upgradeReq property, so use req instead websockets (ws) removed socket.upgradeReq, so use req instead Jun 26, 2017
@ndelangen ndelangen deleted the rn-storybook-websocket-url branch July 8, 2017 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants