hapi icon indicating copy to clipboard operation
hapi copied to clipboard

Update decorator types to be correct

Open feedmypixel opened this issue 1 year ago • 4 comments

  • 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 #4524

feedmypixel avatar Aug 26 '24 17:08 feedmypixel

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

feedmypixel avatar Aug 26 '24 17:08 feedmypixel

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

feedmypixel avatar Aug 27 '24 08:08 feedmypixel

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

damusix avatar Sep 05 '24 04:09 damusix

@damusix Cheers - will look into this ASAP

feedmypixel avatar Sep 10 '24 13:09 feedmypixel

@feedmypixel Any updates on this?

damusix avatar Nov 28 '24 06:11 damusix

@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 avatar Dec 04 '24 09:12 feedmypixel

@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 avatar Dec 04 '24 09:12 feedmypixel

Perhaps this helps: @feedmypixel https://www.typescriptlang.org/play/?#code/C4TwDgpgBAIhDGB7ATgQ2BAcqgttAvFAM7DICWAdgOZQA+xIOARogDYDcAUJ6JFAEoQiEZADcIAE0EBHAK5DgAaQggiUQgApOUHVADkwsSL11tuvbOSsTtMzr1yRIG3f1h0ACxe79eYB8QJb3McMjxg+2FgAFUrCP0ogFkIf0D4vQ8IVAkRInSyINMfPVQwMHSwVlkqSjyi82REWQx01GavevswZAgKnsEiMEQKYTrbYsoAM0R8ogBJCgArBAxC8fMUMioK1DQcMdc9dz2iAEFkNGdOt1QQVkRs9JJ0Xuu9HsHh4XS0AHd0iSIHCoSjpe7bN7gg7FKgQCgiF4DIYjXqcACUXB44GgAFEAB7wCBgYDYPAAHgAKgA+dRQClQCB4jAUCRqAYicRSCCOEjKVRQAD8UHh4mQUAAXHTMZxJrIKPBgGRhlAckg0BgoJSGUy4azYAgUC9SRAqRpeBBJe9ufISHoADRQbqISDIUCS-GE4nGylUh1+AISSUaAB0od2VElqAoIAA2gBdNHqGlRkCJgDeAF9uAB6bN0jxkNSF4WIYBQVBQD4cyRQADWKl+KAknFVhowGitPOA9v0FEQVAAtFCexpE-gaZmMTm8xSC0W1C2DeqIB2el2e3phw7R0moJP2EA

damusix avatar Dec 06 '24 04:12 damusix

@damusix wonderful thanks 👍🏻

feedmypixel avatar Dec 10 '24 10:12 feedmypixel

Continued in https://github.com/hapijs/hapi/pull/4538

damusix avatar Jan 19 '25 06:01 damusix