Page MenuHomePhabricator

[native] Unsubscribe listener on unmount
ClosedPublic

Authored by tomek on May 8 2023, 9:32 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 13, 7:56 PM
Unknown Object (File)
Tue, Nov 12, 11:52 AM
Unknown Object (File)
Mon, Oct 28, 1:26 AM
Unknown Object (File)
Mon, Oct 28, 1:26 AM
Unknown Object (File)
Mon, Oct 28, 1:26 AM
Unknown Object (File)
Mon, Oct 28, 1:26 AM
Unknown Object (File)
Mon, Oct 28, 1:26 AM
Unknown Object (File)
Mon, Oct 28, 1:25 AM
Subscribers

Details

Summary

In D7525 I forgot to include code that unsubscribes on unmount - this may cause some issues when a user logs out, then logs in and clicks a link.

Test Plan

Log out, log in, click a link - it should work.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.May 8 2023, 9:47 AM
Harbormaster failed remote builds in B19138: Diff 26203!
tomek requested review of this revision.May 9 2023, 2:29 AM
native/navigation/invite-link-handler.react.js
39 ↗(On Diff #26248)

I'm not sure how the situation looks now, but I remember Hermes had problems with this construct, hooks weren't firing properly and I had lots of trouble debugging this. The solution was to use curly braces instead of a void-returning expression.

Could you check if this works correctly now? By e.g.

const foo = () => { console.log('unmount'); };

// in hook
return () => foo();

Or simply apply the suggestion without investigation, to save time

native/navigation/invite-link-handler.react.js
39 ↗(On Diff #26248)

Is there a reason we can't just return subscription.remove? Is this not bound for it?

native/navigation/invite-link-handler.react.js
39 ↗(On Diff #26248)

Is there a reason we can't just return subscription.remove? Is this not bound for it?

Yes, Flow is complaining

Cannot get subscription.remove because property remove [1] cannot be unbound from the context [2] where it was defined. [method-unbinding]

@bartek I'm applying your suggestion. Wondering what were the issues you've faced.

Update effect cleanup function

bartek added inline comments.
native/navigation/invite-link-handler.react.js
39 ↗(On Diff #26248)

Should probably work too

This revision is now accepted and ready to land.May 16 2023, 4:16 AM
This revision was automatically updated to reflect the committed changes.