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

v16: look into compatibility with node 10 #3899

Closed
AdriVanHoudt opened this issue Jan 9, 2019 · 7 comments
Closed

v16: look into compatibility with node 10 #3899

AdriVanHoudt opened this issue Jan 9, 2019 · 7 comments
Assignees
Labels
Milestone

Comments

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Jan 9, 2019

@hueniverse you stated that no version is compatible with node 10 and I saw that node 10 was removed from Travis for v16.
We want to move to node 10 but are still on v16 so I thought I could look into this.
From what I can tell there are some deprecation warnings for Buffer usage in the tests which are easy to solve.
Other than that there is 1 test failing on node 10.
Test 807) transmission transmit() does not leak stream data when request timeouts before stream drains
It times out due to it waiting for the stream end event to fire.
https://github.com/hapijs/hapi/blob/v16/test/transmit.js#L1944-L1986
In node 8 it does. In node 10 close is being fired but not end.
I don't know enough of streams to be sure why this is happening but according to the docs The 'end' event will not be emitted unless the data is completely consumed. which seems to me that node 10 is doing the correct thing? (https://nodejs.org/docs/latest-v10.x/api/stream.html#stream_event_end)

What was the result you received?

test 807) transmission transmit() does not leak stream data when request timeouts before stream drains timing out

What did you expect?

All tests passing

Context

  • node version: 10
  • hapi version: 16
  • os: macOS
  • any other relevant information:

Are you ok with an effort to support node 10 for v16? Are there issues for node 10 I might be missing?

@AdriVanHoudt
Copy link
Contributor Author

@AdriVanHoudt AdriVanHoudt commented Jan 9, 2019

It seems that master has an updated version of the test
https://github.com/hapijs/hapi/blob/master/test/transmit.js#L1310-L1344
But unless I'm missing something that test is missing an await team.work to make sure the end event is being called?

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Jan 10, 2019

Ensuring v16 works correctly with node v10 is a bit more effort than just fixing a test. It means testing every dependency as well as other benchmarks. It is the kind of effort I am more likely to take with the new licensed LTS branch at the moment.

You can still test it yourself and if you identify issues that can be fixed with a simple PR to the v16 branch, I can review as time permits.

@hueniverse hueniverse added the lts label Jan 10, 2019
@AdriVanHoudt
Copy link
Contributor Author

@AdriVanHoudt AdriVanHoudt commented Jan 11, 2019

Sounds good 👌
I think we need to start with #3900, which after resolving I can probably port to v16. Together with the other buffer stuff in the tests we can get the tests passing on node 10 at least.
Are the benchmarks you talk about just for hapi or also for individual modules?

(side note: we might be moving to 17 soon-ish, but if I can help people on LTS with this I will gladly try to help get at least test passing on node 10.)

@WesTyler
Copy link
Contributor

@WesTyler WesTyler commented Mar 7, 2019

Given the impending demise of v16 in #3932 , can this be closed?

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Mar 8, 2019

Not yet. It might still happen for the commercial license option.

@hueniverse hueniverse self-assigned this Mar 25, 2019
@hueniverse hueniverse added this to the 16.8.0 milestone Mar 25, 2019
@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Mar 25, 2019

Fixed in #3933

@hueniverse hueniverse closed this Mar 25, 2019
@lock
Copy link

@lock 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
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants