-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Update decorator types to be correct #4525
base: master
Are you sure you want to change the base?
Conversation
@kanongil when you have a min let me know your thoughts on this. Thanks 🙏 |
47f9357
to
1d95f23
Compare
- The code currently allows any value to be added as a decorator, but the types do not. This work allows any value to be added to a decorator by updating the types to reflect what the code does - Add relevant tests - Update inline docs - Create and export DecorationValue type - Fixes hapijs#4524
1d95f23
to
5fc56c4
Compare
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.
This implementation will remove the typing from an inline method declaration, as X | any
will be reduced to any
. I believe it should work better if additional declarations for decorate
are added with method: any
(or maybe even value: any
).
Understood. I'll have a look at adding other value specific overloads. I was also wondering on changing the name of the arg to signify that any |
These extra definitions specify how to use value instead of method
@feedmypixel Agreed with Gil. Perhaps you can type this to be a value from https://github.com/hapijs/hapi/blob/master/lib/request.js#L18 Can you also add type tests? This should guarantee that nothing breaks, and you can see it in your editor in the tests if your intended types are producing https://github.com/hapijs/hapi/blob/master/test/types/index.ts This is a good fix, though. I'm always typing around it with |
@damusix Cheers - will look into this ASAP |
DecorationValue
type