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

Drop node v12 support #4346

Merged
merged 3 commits into from May 16, 2022
Merged

Drop node v12 support #4346

merged 3 commits into from May 16, 2022

Conversation

devinivy
Copy link
Member

Dropping node v12 support. There's a good discussion in #4279 that outlines other steps we may want to take as we end support for node v12. Here's my approach here:

  • I adopted nullish coalescing and optional chaining.
  • Recognizing that it might break code that relies on the old behavior, I did adopt ?? in the place of || when evaluating non-boolean expressions. I am open to other thoughts, but I am of the mind that it usually gives a better, more precise behavior than ||. In some cases I kept || when I thought it made sense, e.g to fall through empty header values of ''.
  • I did not address the usage of private methods since hapi treats private methods as private to hapi and not just the class in question. One great example is request._log() which is called all over the place. I feel that we should adopt private methods in a given class only if we're willing to remove usage of _-prefixed methods from that class that may be used by other classes.
  • I addressed usage of any deprecated APIs I could find: res.writeableEnd rather than res.finished, req.socket rather than req.connection.
  • I did not implement anything with AbortSignal since I didn't want to make any substantive API changes, though I am in support of it.

This PR will not pass CI until hapijs/shot#144 and hapijs/lab#1036 are published. I intend to publish beta versions of these (and other) modules in the near future.

Resolves #4279

@devinivy devinivy added the breaking changes Change that can breaking existing code label Apr 18, 2022
@devinivy devinivy added this to the v21.0.0 milestone Apr 18, 2022
@devinivy
Copy link
Member Author

devinivy commented Apr 25, 2022

I was able to pull in those beta versions of lab, code, and shot. We officially have a passing build of hapi on node v18 :)

I ran into a little snag along the way that is now fixed. Since we've started using req.socket in lieu of the deprecated req.connection, adding req.socket to shot actually put us down some new codepaths in hapi. Previously these code paths were exercised by shot injection requests, which were always missing req.socket. As a result I needed to make shot's req.socket more true to the Net.Socket interface (hapijs/shot#145), and I needed to add a new test to hapi for when req.socket is missing.

@geek
Copy link
Member

geek commented May 9, 2022

Looks good! I tested on a few of the main plugins as well, no issues.

@devinivy devinivy merged commit a034673 into v21 May 16, 2022
@devinivy devinivy deleted the drop-node-12 branch May 16, 2022 19:43
@devinivy devinivy mentioned this pull request Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking changes Change that can breaking existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants