minwork / use-long-press

React hook for detecting click (or tap) and hold event
MIT License
122 stars 12 forks source link

Allow context type override #41

Closed Peeterush closed 1 year ago

Peeterush commented 1 year ago

The context property is of type unknown. Setting a custom context type should be possible as such:

type MyType = { a: string, b: boolean };
const bind = useLongPress<Element, LongPressCallback<Element, MyType>, MyType>(
  (event, { context }) => { /* context should have type 'MyType' */ }
);

But this fails:

Type 'LongPressCallback<Element, { a: string, b: boolean }>' does not satisfy the constraint 'LongPressCallback<Element, unknown>'.
Type 'unknown' is not assignable to type '{ a: string, b: boolean }'.

By setting the useLongPress Callback type to include any without changing its default value, the context type is overwritable while keeping the default unknown when no type parameters are specified.

codecov-commenter commented 1 year ago

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage :thumbsup:

Coverage data is based on head (b736677) compared to base (5ce100b). Patch has no changes to coverable lines.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #41 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 2 2 Lines 82 82 Branches 30 30 ========================================= Hits 82 82 ``` | [Impacted Files](https://codecov.io/gh/minwork/use-long-press/pull/41?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [src/index.ts](https://codecov.io/gh/minwork/use-long-press/pull/41?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2luZGV4LnRz) | `100.00% <ø> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

minwork commented 1 year ago

Hey, thanks for the PR and taking notice on those typings. Although any solves this problem there is even a better solution - passing Context, see #42

Peeterush commented 1 year ago

Forcing the bind and the callback to use the same context type, that's a good improvement!

Would you also consider having Context as the first type parameter? I think most people are not that interested in setting a different Target or Callback type, but we need to supply them to reach Context. Having it first would simply allow useLongPress<MyType>(/* ... */).

Peeterush commented 1 year ago

Now you can also just suppy a Context and a Target type, and let TS figure out the Callback type!

minwork commented 1 year ago

Yes, I was considering it but that would require to bump major version because it would break backward compatibility. I was also considering handling it on TS overloads but it would get ugly. I will do it for sure in the future, but for now will stick with the Context passing fix.

minwork commented 1 year ago

Closing as it was resolved in #42 and future improvements are addressed in #43