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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2d689d6
RN: Add error handler on WS to fix crashing on page reload
Hypnosphi Feb 16, 2018
f46a637
Revert client changes as they aren't needed
Hypnosphi Feb 16, 2018
2a9e3f1
Merge branch 'master' into fix-rn-ws
danielduan Feb 20, 2018
dba45e5
Merge branch 'master' into fix-rn-ws
Hypnosphi Feb 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions app/react-native/src/server/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export default class Server {
this.expressApp.use(storybook(options));
this.httpServer.on('request', this.expressApp);
this.wsServer = new ws.Server({ server: this.httpServer });
// see https://github.com/websockets/ws/issues/1256#issuecomment-364996586
this.wsServer.on('connection', (s, req) => this.handleWS(s, req));
}

Expand All @@ -24,6 +25,12 @@ export default class Server {
}
}

socket.on('error', err => {
// Ignore network errors like `ECONNRESET`, `EPIPE`, etc.
if (err.errno) return;
throw err;
});

socket.on('message', data => {
this.wsServer.clients.forEach(c => {
if (!this.options.manualId || (socket.pairedId && socket.pairedId === c.pairedId)) {
Expand Down
2 changes: 2 additions & 0 deletions lib/channel-websocket/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

logger.error('websocket: connection error', e.message);
};
this._socket.onclose = e => {
Expand Down