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 🙏

@feedmypixel feedmypixel force-pushed the update-decorator-types branch from 47f9357 to 1d95f23 Compare August 26, 2024 17:35
- 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
@feedmypixel feedmypixel force-pushed the update-decorator-types branch from 1d95f23 to 5fc56c4 Compare August 26, 2024 19:03
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
@feedmypixel feedmypixel requested a review from kanongil August 28, 2024 07:42
@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

@damusix
Copy link
Contributor

damusix commented Nov 28, 2024

@feedmypixel Any updates on this?

@feedmypixel
Copy link
Author

@feedmypixel Any updates on this?

Sorry I haven't had time to get to it. I'm hopeful of having some time over the Christmas period

@feedmypixel
Copy link
Author

@damusix to pre-empt me getting to this. A few questions:

@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:

Any thoughts/examples of the type of utility your thinking of?

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

No probs 👍

@feedmypixel
Copy link
Author

@damusix wonderful thanks 👍🏻

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