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

Update decorator types to be correct #4525

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

feedmypixel
Copy link

@feedmypixel feedmypixel commented Aug 26, 2024

  • 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 Incorrect Types for decorators #4524

@feedmypixel
Copy link
Author

@kanongil when you have a min let me know your thoughts on this. Thanks 🙏

- 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
Copy link
Contributor

@kanongil kanongil left a 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).

@feedmypixel
Copy link
Author

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 value can be passed. I'll refactor and push a commit later today

These extra definitions specify how to use value instead of method
@damusix
Copy link
Contributor

damusix commented Sep 5, 2024

@feedmypixel Agreed with Gil. Perhaps you can type this to be a value from Request | Toolkit | Server interface? I imagine it'd be some utility type that removes internal, reserved keys so that it can pickup whatever you extend via declare module:

https://github.com/hapijs/hapi/blob/master/lib/request.js#L18
https://github.com/hapijs/hapi/blob/master/lib/toolkit.js#L11
https://github.com/hapijs/hapi/blob/master/lib/response.js#L25
https://github.com/hapijs/hapi/blob/master/lib/server.js#L174

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 any, errors, or not. Feel free to add missing tests related to this as well. It's your place to verify your changes.

https://github.com/hapijs/hapi/blob/master/test/types/index.ts

This is a good fix, though. I'm always typing around it with X as never.

@feedmypixel
Copy link
Author

@damusix Cheers - will look into this ASAP

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

Successfully merging this pull request may close these issues.

Incorrect Types for decorators
3 participants