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

Improve websocket error handling and fix #315 #320

Merged
merged 14 commits into from
Dec 18, 2017
Merged

Improve websocket error handling and fix #315 #320

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
fedbd52
Merge pull request #1 from parcel-bundler/master
Dec 9, 2017
3f31fc5
Merge branch 'master' of github.com:DeMoorJasper/parcel
DeMoorJasper Dec 9, 2017
b09d15f
Merge pull request #4 from parcel-bundler/master
Dec 11, 2017
80b3431
Merge branch 'master' of github.com:DeMoorJasper/parcel
DeMoorJasper Dec 11, 2017
392bbc9
run prettier
DeMoorJasper Dec 11, 2017
e616cc2
Merge pull request #7 from parcel-bundler/master
Dec 12, 2017
1d90425
stuff
DeMoorJasper Dec 12, 2017
51bfe22
Merge pull request #9 from parcel-bundler/master
Dec 15, 2017
8ad9919
Merge branch 'master' of github.com:DeMoorJasper/parcel
DeMoorJasper Dec 15, 2017
f0b53e2
add proper error handling to sending data
DeMoorJasper Dec 17, 2017
69e4d36
replace hacky bugfix with gracefull connection closing on client side
DeMoorJasper Dec 17, 2017
0a6dc26
improve error handling to catch all errors not just when sending
DeMoorJasper Dec 17, 2017
f125696
fix tests
DeMoorJasper Dec 17, 2017
e01ff63
code cleanup
DeMoorJasper Dec 17, 2017
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
25 changes: 18 additions & 7 deletions .github/ISSUE_TEMPLATE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ Search open/closed issues before submitting since someone might have asked the s
<!--- Provide a general summary of the issue in the title above -->

### 🎛 Configuration (.babelrc, package.json, cli command)

<!--- If describing a bug, tell us what your babel configuration looks like -->

```js
Expand All @@ -18,31 +19,41 @@ Search open/closed issues before submitting since someone might have asked the s
```

### 🤔 Expected Behavior

<!--- If you're describing a bug, tell us what should happen -->

<!--- If you're suggesting a change/improvement, tell us how it should work -->

### 😯 Current Behavior

<!--- If describing a bug, tell us what happens instead of the expected behavior -->

<!--- If you are seeing an error, please include the full error message and stack trace -->

<!--- If suggesting a change/improvement, explain the difference from current behavior -->

### 💁 Possible Solution

<!--- Not obligatory, but suggest a fix/reason for the bug, -->

<!--- or ideas how to implement the addition or change -->

### 🔦 Context

<!--- How has this issue affected you? What are you trying to accomplish? -->

<!--- Providing context helps us come up with a solution that is most useful in the real world -->

### 🌍 Your Environment

<!--- Include as many relevant details about the environment you experienced the bug in -->

| Software | Version(s)
| ---------------- | ----------
| Parcel |
| Node |
| npm/Yarn |
| Operating System |
| Software | Version(s) |
| ---------------- | ---------- |
| Parcel |
| Node |
| npm/Yarn |
| Operating System |

<!-- Love parcel? Please consider supporting our collective:
👉 https://opencollective.com/parcel/donate -->
👉 https://opencollective.com/parcel/donate -->
7 changes: 1 addition & 6 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,29 +35,24 @@ yarn test
[node]: https://nodejs.org/
[yarn]: https://yarnpkg.com/


## Financial contributions

We also welcome financial contributions in full transparency on our [open collective](https://opencollective.com/parcel).
Anyone can file an expense. If the expense makes sense for the development of the community, it will be "merged" in the ledger of our open collective by the core contributors and the person who filed the expense will be reimbursed.


## Credits


### Contributors

Thank you to all the people who have already contributed to parcel!
<a href="graphs/contributors"><img src="https://opencollective.com/parcel/contributors.svg?width=890" /></a>


### Backers

Thank you to all our backers! [[Become a backer](https://opencollective.com/parcel#backer)]

<a href="https://opencollective.com/parcel#backers" target="_blank"><img src="https://opencollective.com/parcel/backers.svg?width=890"></a>


### Sponsors

Thank you to all our sponsors! (please ask your company to also support this open source project by [becoming a sponsor](https://opencollective.com/parcel#sponsor))
Expand All @@ -71,4 +66,4 @@ Thank you to all our sponsors! (please ask your company to also support this ope
<a href="https://opencollective.com/parcel/sponsor/6/website" target="_blank"><img src="https://opencollective.com/parcel/sponsor/6/avatar.svg"></a>
<a href="https://opencollective.com/parcel/sponsor/7/website" target="_blank"><img src="https://opencollective.com/parcel/sponsor/7/avatar.svg"></a>
<a href="https://opencollective.com/parcel/sponsor/8/website" target="_blank"><img src="https://opencollective.com/parcel/sponsor/8/avatar.svg"></a>
<a href="https://opencollective.com/parcel/sponsor/9/website" target="_blank"><img src="https://opencollective.com/parcel/sponsor/9/avatar.svg"></a>
<a href="https://opencollective.com/parcel/sponsor/9/website" target="_blank"><img src="https://opencollective.com/parcel/sponsor/9/avatar.svg"></a>
6 changes: 2 additions & 4 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@
```shell
yarn global add parcel-bundler
```

or with npm:

```shell
npm install -g parcel-bundler
```
Expand Down Expand Up @@ -97,14 +99,12 @@ All feedback and suggestions are welcome!
This project exists thanks to all the people who contribute. [[Contribute]](CONTRIBUTING.md).
<a href="graphs/contributors"><img src="https://opencollective.com/parcel/contributors.svg?width=890" /></a>


## Backers

Thank you to all our backers! 🙏 [[Become a backer](https://opencollective.com/parcel#backer)]

<a href="https://opencollective.com/parcel#backers" target="_blank"><img src="https://opencollective.com/parcel/backers.svg?width=890"></a>


## Sponsors

Support this project by becoming a sponsor. Your logo will show up here with a link to your website. [[Become a sponsor](https://opencollective.com/parcel#sponsor)]
Expand All @@ -120,8 +120,6 @@ Support this project by becoming a sponsor. Your logo will show up here with a l
<a href="https://opencollective.com/parcel/sponsor/8/website" target="_blank"><img src="https://opencollective.com/parcel/sponsor/8/avatar.svg"></a>
<a href="https://opencollective.com/parcel/sponsor/9/website" target="_blank"><img src="https://opencollective.com/parcel/sponsor/9/avatar.svg"></a>



## License

MIT
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
"uglify-es": "^3.2.1",
"v8-compile-cache": "^1.1.0",
"worker-farm": "^1.4.1",
"ws": "^3.3.2"
"ws": "^3.3.3"
},
"devDependencies": {
"babel-cli": "^6.26.0",
Expand Down
12 changes: 12 additions & 0 deletions src/HMRServer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,14 @@ class HMRServer {
});

this.wss.on('connection', ws => {
ws.onerror = this.handleSocketError;
if (this.unresolvedError) {
ws.send(JSON.stringify(this.unresolvedError));
}
});

this.wss.on('error', this.handleSocketError);
Copy link

Choose a reason for hiding this comment

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

This is probably not needed as it adds the listener after the server is bound. errors are forwarded from the internal http.Server and most of them (all?) are emitted when listening.


return this.wss._server.address().port;
}

Expand Down Expand Up @@ -69,6 +72,15 @@ class HMRServer {
}
}

handleSocketError(err) {
if (err.code === 'ECONNRESET') {
// This gets triggered on page refresh, ignore this
return;
}
// TODO: Use logger to print errors
console.log(prettyError(err));
Copy link
Member

Choose a reason for hiding this comment

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

have you seen any other errors actually happen? should we just ignore them?

Copy link
Member Author

@DeMoorJasper DeMoorJasper Dec 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This was just for future error handling.
Normally ECONNRESET is the only error that accures due to chrome not properly closing the connection

Copy link

Choose a reason for hiding this comment

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

Other errors might be parser errors, like an invalid frame, an inflate error if you use permessage-deflate, a frame with a payload bigger than the maxPayload option or other net.Socket errors like write EPIPE, write ECONNABORTED, etc.

}

broadcast(msg) {
const json = JSON.stringify(msg);
for (let ws of this.wss.clients) {
Expand Down
5 changes: 4 additions & 1 deletion src/builtins/hmr-runtime.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,10 @@ if (!module.bundle.parent) {
}

if (data.type === 'reload') {
window.location.reload();
ws.close();
ws.onclose = () => {
window.location.reload();
}
}

if (data.type === 'error-resolved') {
Expand Down
21 changes: 15 additions & 6 deletions test/hmr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,24 @@ describe('hmr', function() {
rimraf.sync(__dirname + '/input');
});

afterEach(function() {
if (b) {
b.stop();
b = null;
}
afterEach(function(done) {
let finalise = () => {
if (b) {
b.stop();
b = null;

done();
}
};

if (ws) {
ws.close();
ws = null;
ws.onclose = () => {
ws = null;
finalise();
};
} else {
finalise();
}
});

Expand Down
4 changes: 3 additions & 1 deletion test/plugins.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ describe('plugins', function() {
});

it('should load package.json from parent tree', async function() {
let b = await bundle(__dirname + '/integration/plugins/sub-folder/index.js');
let b = await bundle(
__dirname + '/integration/plugins/sub-folder/index.js'
);

assertBundleTree(b, {
name: 'index.js',
Expand Down
6 changes: 3 additions & 3 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4934,9 +4934,9 @@ write-file-atomic@^1.1.4:
imurmurhash "^0.1.4"
slide "^1.1.5"

ws@^3.3.2:
version "3.3.2"
resolved "https://registry.yarnpkg.com/ws/-/ws-3.3.2.tgz#96c1d08b3fefda1d5c1e33700d3bfaa9be2d5608"
ws@^3.3.3:
version "3.3.3"
resolved "https://registry.yarnpkg.com/ws/-/ws-3.3.3.tgz#f1cf84fe2d5e901ebce94efaece785f187a228f2"
dependencies:
async-limiter "~1.0.0"
safe-buffer "~5.1.0"
Expand Down