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

Custom error cloning to fix node v21 issue #390

Merged
merged 2 commits into from
Oct 23, 2024
Merged

Conversation

kanongil
Copy link
Contributor

@kanongil kanongil commented Oct 30, 2023

This PR fixes a Hoek.clone(err) issue introduced in node v21.

The problem

Node 21 updates the V8 engine to 11.8, which includes a patch from v11.5.1 that changes how the Error.stack property works.

With this patch, a copied err.stack descriptor will no longer return the stack of the source error, but always return undefined. This breaks Boom, where it relies on clone() to fully work.

This manifestation of this issue was reported on discord.

The solution

The only way to copy a working stack descriptor in node 21, is to used the structuredClone() method. This is a platform method that uses an algorithm from the HTML spec to create a "clone". Only the "name", "message", "cause", and "stack" properties are transferred, and the prototype is set to a standard Error. See the serialization algorithm step 17 which deals with [[ErrorData]] objects.

I use structuredClone() to create the base object, and then pass it through the standard recursive object cloner to clone all properties except the stack property. This preserves the existing behaviour, including for weird corner cases.

This PR will only work with node >= v17.0.0, which is when structuredClone() was added. Given that every node release <18 is no longer supported, I don't want to spend energy fixing this, and expect that this fix can be part of a new breaking change release. Or is the current release expected to support node v21?

@kanongil kanongil added bug Bug or defect breaking changes Change that can breaking existing code labels Oct 30, 2023
@Marsup
Copy link
Contributor

Marsup commented Oct 30, 2023

Should we then do node version detection? I don't think hapi 22 is anywhere near.

@Nargonath
Copy link
Member

I agree with @Marsup, AFAIK hapi@22 is not going to see the light soon. If we want to deploy this fix faster, keeping a retro-compatibility might be best especially since this fix is only needed for >= node@21 AFAIU.

@kanongil
Copy link
Contributor Author

It's simple enough to support both with a few if (typeof structuredClone === 'function') { … }, falling back to the current handling. It gets a bit more complicated regarding the test code coverage, which can only enter one of the branches.

@kanongil
Copy link
Contributor Author

kanongil commented Nov 6, 2023

IMO, if no significant breaking changes to functionality are pending, hapi and its dependencies should do a version bump to deprecate node v14 and v16 and support v20. It would be nice to finally merge hapijs/hapi#4433.

FYI, the hapi test suite fails some tests on node v20 (at least on macOS).

@kanongil kanongil changed the base branch from master to next November 28, 2023 11:18
@lenovouser
Copy link
Contributor

We are experiencing this as well, could this be merged?

@Marsup
Copy link
Contributor

Marsup commented Oct 23, 2024

@kanongil I'm unclear as to the status of this one, do you still consider it a breaking change or were the latest commits enough to make it compatible? I'm not holding this one back, just thinking about a possible backport.

@Marsup Marsup merged commit 98652b0 into hapijs:next Oct 23, 2024
9 checks passed
@kanongil
Copy link
Contributor Author

It's only breaking due to the v14/v16 incompatibility.

A backport can work, if it is updated to retain the current behaviour on node <18 as mentioned in #390 (comment).

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 bug Bug or defect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants