-
Notifications
You must be signed in to change notification settings - Fork 171
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
Support the URL type #392
Conversation
This fixes that particular symptom but isn't it an indicator of a problem with the current cloning approach at large? |
There was a problem hiding this 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
I understand your concern @voxpelli. I read your comment also in the other issue (hapijs/wreck#305 (comment)) regarding typing the options as |
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). |
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. |
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. |
is this instead of the nowadays standardized |
@voxpelli Not quite. Boom will convert a passed error to a Boom error, which is mostly done by adding an It could make sense to update this approach to a simpler: Create new Boom error with |
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 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. |
@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. |
The new version has been released as |
Great. Any reason #389 was not merged for it as well? It is sorely missing for typescript support. |
It's just an oversight on my part, my bad. |
I added support for the
URL
type, both inHoek.deepEqual()
andHoek.clone()
.This fixes the issue in hapijs/wreck#305 (comment).