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

Node 8 fixes #3507

Merged
merged 2 commits into from
Jun 4, 2017
Merged

Node 8 fixes #3507

merged 2 commits into from
Jun 4, 2017

Conversation

kanongil
Copy link
Contributor

@kanongil kanongil commented Jun 2, 2017

Node 8 changes the stream.Readable implementation a bit. Some of the tests expects the .destroy() method to be absent, which I fixed by always clearing it.

The more serious issue, is server.inject(), which can return incorrect headers. This is not a test issue, but rather a bug, which is fixed by updating to the latest shot release.

Closes #3510

@cjihrig
Copy link
Contributor

cjihrig commented Jun 2, 2017

Node core has a testing utility named Canary in the Gold Mine (CITGM) that is used to detect npm ecosystem breakage. It runs the regression tests of many popular modules, but hapi is no longer one of them. @hueniverse, you might want to work with the CITGM team to get hapi tested again.

Sorry to hijack this PR.

@DavidTPate
Copy link

@kanongil For my own curiosity can you point me in the direction of the tests that are expecting the destroy method to not be present? I'm guessing that you're referring to response.

If we have a part causing these tests to fail with Node 8 instead of just removing the destroy() method for streams I would expect us to fix the underlying code to prevent compatibility issues with Node 8.

@kanongil
Copy link
Contributor Author

kanongil commented Jun 2, 2017

@DavidTPate Don't worry about that part. The tests exercises code that drains streams with no destroy method. I fixed it by removing the destroy method from the test stream. I could also have done it by using an old streams2 Readable.

@kanongil kanongil closed this Jun 2, 2017
@kanongil kanongil reopened this Jun 2, 2017
@AdriVanHoudt
Copy link
Contributor

it looks like hapi has the same errors on node 6 https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-nobuild/150/#showFailuresLink

@DavidTPate DavidTPate mentioned this pull request Jun 4, 2017
1 task
@hueniverse hueniverse added the dependency Update module dependency label Jun 4, 2017
@hueniverse hueniverse added this to the 16.3.1 milestone Jun 4, 2017
@hueniverse hueniverse merged commit 8a92e60 into hapijs:master Jun 4, 2017
@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dependency Update module dependency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update hapijs/shot to 3.4.2 from 3.4.0
5 participants