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

Support the URL type #392

Merged
merged 1 commit into from
Dec 4, 2023
Merged

Support the URL type #392

merged 1 commit into from
Dec 4, 2023

Conversation

kanongil
Copy link
Contributor

I added support for the URL type, both in Hoek.deepEqual() and Hoek.clone().

This fixes the issue in hapijs/wreck#305 (comment).

@voxpelli
Copy link

This fixes that particular symptom but isn't it an indicator of a problem with the current cloning approach at large?

Copy link
Member

@Nargonath Nargonath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work @kanongil. LGTM

@Nargonath
Copy link
Member

I understand your concern @voxpelli. I read your comment also in the other issue (hapijs/wreck#305 (comment)) regarding typing the options as readonly but that would work only for people actively using TypeScript. I haven't benchmarked the implications of using Object.freeze so I cannot really talk about its impact. As you can see from the code, it's not the first type that was handled this way. I think we prefer to have some special handlings for specific types to allow cloning rather than having to hunt down bugs where people mutated their parameters somewhere in their applications where they shouldn't have.

@ljharb
Copy link

ljharb commented Nov 29, 2023

That's kind of on them, though - not every kind of thing can be cloned, and you can't plan for userland types - freezing also doesn't help because it doesn't lock down internal state.

Generally this kind of thing is handled by simply not mutating anything, and teaching users to do the same (but if they DO mutate, that's what they've chosen).

@Nargonath
Copy link
Member

Nargonath commented Nov 29, 2023

Sure, I understand your point. You still have to spend time either to educate your users or to diagnose a bunch of issues that stem from mutation problems which might not be easily identifiable. IMO I prefer to clone pre-emptively to avoid these kind of problems. It's probably a matter of personal opinion at this point.

@ljharb
Copy link

ljharb commented Nov 29, 2023

Sure, you can choose either approach. However, since the preemptive cloning is impossible in a generic sense, in my experience the tradeoffs (like this one, which was a hard to debug error for the user) are better with the education choice (especially considering most of the ecosystem now prefers to avoid mutation).

@kanongil
Copy link
Contributor Author

Sure, you can choose either approach. However, since the preemptive cloning is impossible in a generic sense, in my experience the tradeoffs (like this one, which was a hard to debug error for the user) are better with the education choice (especially considering most of the ecosystem now prefers to avoid mutation).

It might make sense to revisit the approach hapi takes to cloning options. Javascript programming has definitely changed in the years since this approach was decided to be used in hapi.

Anyway, a best effort clone a javascript object method still makes sense for hapi. It's eg. used in Boom to clone a passed error, which is subsequently boomified and returned.

@voxpelli
Copy link

It's eg. used in Boom to clone a passed error, which is subsequently boomified and returned.

is this instead of the nowadays standardized .cause property on Error instances? (Which me and @ljharb coincidentally maintains one of the two large polyfills / ponyfills for)

@kanongil
Copy link
Contributor Author

@voxpelli Not quite. Boom will convert a passed error to a Boom error, which is mostly done by adding an isBoom property along with a data and output property to the object (after deep cloning it).

It could make sense to update this approach to a simpler: Create new Boom error with cause from the passed error.

@Nargonath
Copy link
Member

Alright, seems we might look into removing the cloning where it makes sense in the future. For now, I'll merge this PR and deploy a new version to unblock the initial problem raised.

@Nargonath Nargonath merged commit ac1dc42 into hapijs:master Dec 4, 2023
9 checks passed
@KrayzeeKev
Copy link

@Nargonath How do releases work? This repo hasn't had a release in 3 years (from my reading). Yet my package-lock.json tells me differently.

@Nargonath
Copy link
Member

@KrayzeeKev historically hapi org didn't use the Releases feature from GitHub and has its own way of versioning the projects. We use GitHub milestones and issues to list the changelog when having big releases. You can find the list of milestones here: https://github.com/hapijs/hoek/milestones?state=closed. For the issues with changelog content you filter those with label "release notes": https://github.com/hapijs/hoek/issues?q=is%3Aissue+is%3Aclosed+label%3A%22release+notes%22. Though it appears that we haven't written one for Hoek for a while now. It depends on the project but your best bet is to go through the milestone list. Usually the website reads all of this to produce the changelog page: https://hapi.dev/module/hoek/changelog/. It's outdated right now as we have a build error we need to fix in our CI.

I'll publish Hoek next version today.

@Nargonath Nargonath added bug Bug or defect and removed feature New functionality or improvement labels Dec 5, 2023
@Nargonath Nargonath self-assigned this Dec 5, 2023
@Nargonath Nargonath added this to the 11.0.3 milestone Dec 5, 2023
@Nargonath
Copy link
Member

The new version has been released as v11.0.3.

@kanongil
Copy link
Contributor Author

kanongil commented Dec 5, 2023

Great. Any reason #389 was not merged for it as well? It is sorely missing for typescript support.

@Nargonath
Copy link
Member

It's just an oversight on my part, my bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug or defect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants