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 #4279

Closed
kanongil opened this issue Aug 27, 2021 · 15 comments
Closed

Drop node v12 support #4279

kanongil opened this issue Aug 27, 2021 · 15 comments
Labels
breaking changes Change that can breaking existing code feature New functionality or improvement

Comments

@kanongil
Copy link
Contributor

kanongil commented Aug 27, 2021

Support plan

  • is this issue currently blocking your project? (yes/no): no
  • is this issue affecting a production system? (yes/no): no

Context

  • node version: n/a
  • module version: v20.x
  • environment (e.g. node, browser, native): node
  • used with (e.g. hapi application, another framework, standalone, ...): standalone
  • any other relevant information:

What problem are you trying to solve?

Allow to develop hapi and modules using more current node.js and javascript APIs and features. v12 is in maintenance mode and support ends 2022-04-30.

New JS features we can use:

  1. Optional chaining syntax (?.).
  2. Nullish coalescing operator (a ?? b).
  3. Private class methods (this.#privateMethod()).
  4. WeakRef support.

From node 14 (LTS with v14.15.0) we get:

  1. ESM modules (stable from v14.17.0, but also in v12.22.0).
  2. Top-Level await support.
  3. A more stable stream.pipeline().
  4. Fs.rm() method.

If we require v14.17+, we could also sneak in:

  1. AbortSignal support in node APIs.

Do you have a new or modified API suggestion to solve the problem?

N/A.

@kanongil kanongil added feature New functionality or improvement breaking changes Change that can breaking existing code labels Aug 27, 2021
@kanongil
Copy link
Contributor Author

So I haven't given much credence to this, beyond listing some of the stuff we could possibly use with this. I will try to find some concrete examples of how this can make Hapi better.

The first is that "Top-Level await" will enable simpler code examples in docs and tutorials, as any time the framework needs to be initialized, a wrapper method is currently used to encapsulate the async logic. Eg. the example function here:

hapi/API.md

Lines 1365 to 1390 in c2107e9

const Hapi = require('@hapi/hapi');
async function example() {
const server = Hapi.server({ host: 'localhost', port: 8000 });
// Defines new handler for routes on this server
const handler = function (route, options) {
return function (request, h) {
return 'new handler: ' + options.msg;
}
};
server.decorate('handler', 'test', handler);
server.route({
method: 'GET',
path: '/',
handler: { test: { msg: 'test' } }
});
await server.start();
}

With top-Level await, this wrapper can be removed, making the code easier to follow.

Feel free to come up with other examples where the new features can help.

@kanongil
Copy link
Contributor Author

kanongil commented Aug 28, 2021

"Optional chaining" can simplify existing code wherever we use the pattern object && object.property. This can in most cases be rewritten as the more succinct object?.property. Eg. here:

const plugin = realm && realm.plugin;

@kanongil
Copy link
Contributor Author

"Private class methods" allows us to hide methods from public objects in a simple to use manner.

This means we can better control the public API and plugins can no longer use such private APIs. Eg. we don't risk the private response._header() being called outside of our control.

@kanongil
Copy link
Contributor Author

kanongil commented Aug 28, 2021

"Nullish coalescing" is especially useful when extracting default values from passed options and can often work better than the || operator that is frequently used for this.

Eg. here is an obvious example, where it would actually fix a weird inconsistency when the timeout is 0:

options.timeout = options.timeout || 5000; // Default timeout to 5 seconds

=>

options.timeout = options.timeout ?? 5000;

But this is a tricky change, as the "" and false values would no longer use the default value, but be passed on as-is. This should not be a problem when called from Typescript, but regular JS user might (inadvertently) rely on it.

Btw, the above can be even better written as this, where it would completely skip the assignment when there is a value supplied:

options.timeout ??= 5000;

This is a minor optimization, and potentially avoids a thrown exception when the supplied value is not writable.

Edit: The ??= syntax requires node v16.

@kanongil
Copy link
Contributor Author

kanongil commented Aug 28, 2021

stream.pipeline() could probably be used to simplify the custom pipelining code here:

hapi/lib/transmit.js

Lines 359 to 377 in c2107e9

internals.chain = function (sources) {
let from = sources[0];
for (let i = 1; i < sources.length; ++i) {
const to = sources[i];
if (to) {
from.on('error', internals.errorPipe.bind(from, to));
from = from.pipe(to);
}
}
return from;
};
internals.errorPipe = function (to, err) {
to.emit('error', err);
};

Edit: while it can make the code simpler, it will also require extra processing since stream.pipeline() is very complex and support lots of use-cases.

@Nargonath
Copy link
Member

Thanks for the examples Gil.

I believe Eran already used private class methods somewhere in the org already.

@kanongil
Copy link
Contributor Author

I believe Eran already used private class methods somewhere in the org already.

No, only private instance fields, which is supported in node 12 but was still not fully part of ES spec when we started using it.

Btw, I would guess that private class methods can allow (slightly) more efficient interpretation / compiled code, since V8 can rely on that the methods never changes.

@Nargonath
Copy link
Member

Nargonath commented Aug 30, 2021

No, only private instance fields, which is supported in node 12 but was still not fully part of ES spec when we started using it.

Yeah you're right, my bad. I read your initial comment too fast. 😅

Btw, I would guess that private class methods can allow (slightly) more efficient interpretation / compiled code, since V8 can rely on that the methods never changes.

That would make sense indeed.

@kanongil
Copy link
Contributor Author

I added AbortSignal to the list. It seems like a promising way to simplify / unify aborting async code.

This is not only interesting for simplifying internal processing, but also to extend the hapi API itself. Eg. h could include a new aborted property, which a handler can pass along to node API calls:

handler(request, h) {

    return Util.promisify(Fs.readFile('/some/file', { signal: h.aborted }));
}

Then if a client closes the request before the payload has been fully delivered, hapi can trigger the h.aborted signal, and automatically cause the read to immediately stop (rather than pointlessly continuing to completion).

@devinivy
Copy link
Member

Very cool, thanks for surfacing this.

@devinivy
Copy link
Member

devinivy commented Oct 8, 2021

Another one: we should move to checking res.writableEnded rather than res.finished.

@kanongil
Copy link
Contributor Author

kanongil commented Oct 8, 2021

I did not mention ESM modules. While I think hapi should definitely support these (for user plugins, when imported, and for testing in Lab), I don't think there is an advantage to convert all the implementation to use it.

The place where it makes most sense, is for modules that are likely to be used browsers. It would enable eg. much of Hoek to be imported directly without transpiling.

We should however consider how to best interoperate with it. Eg. by removing default exports in public API's.

@devinivy
Copy link
Member

devinivy commented Oct 8, 2021

I believe ESM may merit a thread of its own. But I personally would like to see hapi provide proper ESM support, and also would like to see it remain written in CJS.

@kanongil
Copy link
Contributor Author

kanongil commented Oct 8, 2021

So how to tackle this?

Most of this can be converted internally without any user-facing changes and essentially only require an updated minimum version of node. Then the actual implementation can be sorted along the way.

There are however a few of the features that could require breaking changes to implement, and would therefore best be included in the release that changes to require node 14. These are:

  • ESM (revising exports from public APIs).
  • Private class methods (technically it shouldn't, but there are likely code that depends on the current semi-private methods).
  • AbortSignal support (depending on whether it makes sense to break existing APIs).

As such, any work on this should probably focus on the above.

@devinivy
Copy link
Member

devinivy commented Nov 7, 2022

Resolved with v21 #4386

@devinivy devinivy closed this as completed 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 feature New functionality or improvement
Projects
None yet
Development

No branches or pull requests

3 participants