Conversation
typescript-bot
added
Popular package
|
@st-sloth Thank you for submitting this PR! 🔔 @alloy @huhuanming @iRoachie @Skn0tt @timwangdev @kamal @alexdunne @swissmanu @bm-software @a-tarasyuk @mvdam @Esemesek @mrnickel @souvik-ghosh @nossbigg @saranshkataria @franzmoro @tykus160 @jakebloom @ceyhun @mcmar @theohdv @TheSavior @romain-faust @bebebebebe @Naturalclar @chinesedfan @vtolochk @SychevSP @RageBill @sasurau4 @256hz @doumart @drmas - please review this PR in the next few days. Be sure to explicitly select If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead. |
typescript-bot
added
Owner Approved
|
A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped! |
| addEventListener(eventName: BackPressEventName, handler: () => void): NativeEventSubscription; | ||
| removeEventListener(eventName: BackPressEventName, handler: () => void): void; | ||
| addEventListener(eventName: BackPressEventName, handler: () => true | void): NativeEventSubscription; | ||
| removeEventListener(eventName: BackPressEventName, handler: () => true | void): void; |
There was a problem hiding this comment.
This change seems incorrect.. Even without this change, you should be able to pass handle that returns true
There was a problem hiding this comment.
Yes, indeed, one is already able to pass a handler that returns anything at all, as no return value is strictly needed. However the handler must return true, as per documentation (or actually any truthy value, as per implementation), to cause specific behaviour - stopping all the subsequent listeners, including the default one, from executing. And it seems appropriate to document that the handler "should" return true or nothing (even though, in the implementation, the return value is typecasted to boolean, and any value is possible).
There was a problem hiding this comment.
The Flow version doesn’t really say anything about return type. Looking at the docs and the implementation, it seems to me like this should just be () => boolean, but perhaps () => boolean | null | undefined is acceptable too.
In any case, it would be good to send a similar change upstream, unless I’m overlooking a reason for this to not be typed as such already. /cc @TheSavior
There was a problem hiding this comment.
I’m not familiar with this. If the flow code doesn’t include boolean then it is likely that we don’t have any callsites doing that, which makes me not trust the documentation. Can you verify the runtime behavior of the listeners and behavior of returning true?
There was a problem hiding this comment.
Yes, the links are attached
- https://github.com/facebook/react-native/blob/8553e1acc4195479190971cc7a3ffaa0ed37a5e0/Libraries/Utilities/BackHandler.android.js#L24
- https://github.com/facebook/react-native/blob/8553e1acc4195479190971cc7a3ffaa0ed37a5e0/Libraries/Utilities/BackHandler.ios.js#L79
There was a problem hiding this comment.
Ah, okay, I understand better now. The flow code for those functions is untyped. Function is the same as any. I think having the type be boolean | void is reasonable. Please upstream a PR making it typed as returning ?boolean.
There was a problem hiding this comment.
I think having the type be boolean | void is reasonable. Please upstream a PR making it typed as returning ?boolean.
Just to be sure we’re all on the same page; I assume you meant the Flow equivalent that I posted: () => boolean | null | undefined. Using void would I think send the wrong message, i.e. that the return value would not lead to effects.
There was a problem hiding this comment.
Correct. I was agreeing with what you posted :-) Thanks for checking
typescript-bot
added
Revision needed
|
@st-sloth One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you! |
|
@st-sloth I haven't seen anything from you in a while and this PR currently has problems that prevent it from being merged. The PR will be closed tomorrow if there aren't new commits to fix the issues. |
typescript-bot
added
the
Abandoned
|
@st-sloth To keep things tidy, we have to close PRs that aren't mergeable but don't have activity from their author. No worries, though - please open a new PR if you'd like to continue with this change. Thank you! |
Summary: Following the comment DefinitelyTyped/DefinitelyTyped#42618 (comment) Modify the flowtype of BackHandler function to have more specific return type. ## Changelog [General] [Changed] - Changed type of BackHandler to be more specific. Pull Request resolved: #28192 Test Plan: No flow error in RNTester Reviewed By: TheSavior Differential Revision: D20771113 Pulled By: hramos fbshipit-source-id: 5ca65e2a2b3f8726b8fb4606473d8fad5b0ce730
5 participants
Please fill in this template.
npm test.)npm run lint package-name(ortscif notslint.jsonis present).Select one of these and delete the others:
If changing an existing definition:
While the docs say that the handler should return
truewhen the handling should stop, in the implementation, any truthy value does that.