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

RN: Add error handler on WS to fix crashing on page reload #3002

Merged
merged 4 commits into from
Feb 20, 2018
Merged

Conversation

Hypnosphi
Copy link
Member

@Hypnosphi Hypnosphi commented Feb 16, 2018

Issue: #2923

What I did

Added error handling according to websockets/ws#1256 (comment)

How to test

Launch CRNA example, refresh desktop browser tab (it should be Chrome)

@Hypnosphi Hypnosphi added bug react-native patch:yes Bugfix & documentation PR that need to be picked to main branch labels Feb 16, 2018
@@ -52,6 +52,8 @@ export class WebsocketTransport {
this._handler(event);
};
this._socket.onerror = e => {
// Ignore network errors like `ECONNRESET`, `EPIPE`, etc.
if (e.errno) return;
Copy link

Choose a reason for hiding this comment

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

This will also catch connection errors like ECONNREFUSED, not sure if wanted.

Copy link
Member Author

@Hypnosphi Hypnosphi Feb 16, 2018

Choose a reason for hiding this comment

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

What would be the proper filter to only catch chrome tab reload?

Copy link

Choose a reason for hiding this comment

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

Is WebSocket here the browser native WebSocket object or ws or it depends on the context?

Copy link

Choose a reason for hiding this comment

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

I think this is the browser WebSocket as per https://github.com/storybooks/storybook/pull/3002/files#diff-432fa23ac718b94ed5be1b2802371d41R3. In this case you don't need this, the fix on the server is sufficient and correct.

@Hypnosphi Hypnosphi requested a review from a team February 16, 2018 22:35
@codecov
Copy link

codecov bot commented Feb 16, 2018

Codecov Report

Merging #3002 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3002      +/-   ##
==========================================
- Coverage    37.3%   37.29%   -0.02%     
==========================================
  Files         435      435              
  Lines        9432     9435       +3     
  Branches      883      896      +13     
==========================================
  Hits         3519     3519              
- Misses       5379     5394      +15     
+ Partials      534      522      -12
Impacted Files Coverage Δ
app/react-native/src/server/index.js 0% <0%> (ø) ⬆️
app/vue/src/server/config/babel.js 0% <0%> (-100%) ⬇️
app/vue/src/server/utils.js 0% <0%> (-100%) ⬇️
app/react/src/server/babel_config.js 0% <0%> (-76.67%) ⬇️
lib/ui/src/modules/ui/components/layout/index.js 29.28% <0%> (ø) ⬆️
app/react-native/src/bin/storybook-build.js 0% <0%> (ø) ⬆️
addons/actions/src/lib/util/typeReviver.js 71.42% <0%> (ø) ⬆️
lib/ui/src/modules/ui/containers/shortcuts_help.js 25% <0%> (ø) ⬆️
addons/knobs/src/angular/utils.js 82.14% <0%> (ø) ⬆️
addons/storyshots/src/angular/renderTree.js 61.9% <0%> (ø) ⬆️
... and 48 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 4741632...dba45e5. Read the comment docs.

@danielduan
Copy link
Member

saw you denied the two screenshot changes. is that because of the async nature of screenshots or is this PR not ready to go yet?

@tmeasday
Copy link
Member

Apologies again about these flaky tests. I hope to have it sorted out in the next 24-48 hours. More than happy to revert the PR that added them in the meantime.

@danielduan
Copy link
Member

@tmeasday does the screenshots happen when document.ready or is there some sort of timer that gets triggered? I think the screenshots here look like they're taken too early.

There was one I just fixed in my other PR where the early blank screenshot was set as the correct test set. I don't think it's a huge issue but maybe your other customers might mind if the timing is off.

@tmeasday
Copy link
Member

@danielduan in this case it is due to the stories rendering an iframe, which we then need to wait to load. You can read more detail about what's happening here: #2958

@Hypnosphi Hypnosphi merged commit 0404c7a into master Feb 20, 2018
@Hypnosphi Hypnosphi deleted the fix-rn-ws branch February 20, 2018 09:08
shilman pushed a commit that referenced this pull request Feb 21, 2018
RN: Add error handler on WS to fix crashing on page reload
@nikhedonia
Copy link

could you someone publish 3.3.14 to npm? - many thanks

@Hypnosphi
Copy link
Member Author

Hypnosphi commented Feb 21, 2018

It's actually published:
https://www.npmjs.com/package/@storybook/react-native

3.3.14 is the latest of 73 releases

If you're behind a corporate proxy, you may need to wait until it updates its cache

@Hypnosphi Hypnosphi added patch:done Patch/release PRs already cherry-picked to main/release branch and removed patch:done Patch/release PRs already cherry-picked to main/release branch labels Mar 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch react-native
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants