Page MenuHomePhabricator

[lib] [feat] [ENG-991] add usePromoteThread hook
ClosedPublic

Authored by benschac on Apr 15 2022, 8:14 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 8, 10:30 AM
Unknown Object (File)
Fri, Nov 8, 3:50 AM
Unknown Object (File)
Tue, Nov 5, 6:01 AM
Unknown Object (File)
Thu, Oct 31, 5:49 AM
Unknown Object (File)
Tue, Oct 29, 12:32 PM
Unknown Object (File)
Tue, Oct 29, 12:32 PM
Unknown Object (File)
Tue, Oct 29, 12:32 PM
Unknown Object (File)
Tue, Oct 29, 12:32 PM

Details

Summary

abstract promote thread into a hook for use in both native and web. We're using the same API call and reducer in web, all we need to do is pull the call in native into lib so we can use it in two places.

Test Plan

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

benschac added inline comments.
lib/hooks/promote-thread.react.js
18 ↗(On Diff #11517)

Looking at the code base, all catch errors have an any type. Assuming this is okay @ashoat.

ashoat requested changes to this revision.Apr 18 2022, 8:32 PM
ashoat added inline comments.
lib/hooks/promote-sidebar.react.js
16–17

Prefer $ReadOnly types! This should be something you're deeply aware of given you're researching covariance/contravariance right now. You will get this comment for every new type you introduce – please try to grok it and integrate it going forward

43
  1. Are you sure e is a string? What sort of errors can the code in the try block throw? Throwing strings is pretty unusual. Usually Errors are thrown.
  2. Is there some strong reason you need a string from the exception? What do you intend to do with the string? If you really need a string, you could use getMessageForException here
This revision now requires changes to proceed.Apr 18 2022, 8:32 PM
lib/hooks/promote-sidebar.react.js
16–17

Sorry! Thanks for catching this. Adding the additional + is pretty subtle and I just forgot. I need to be more aware.

changes aren't reflected

ashoat requested changes to this revision.Apr 19 2022, 12:18 PM
ashoat added inline comments.
lib/hooks/promote-sidebar.react.js
22 ↗(On Diff #11615)
  1. What are we doing with the Error? Why do we need to pass it in at all?
  2. Do you feel like you can safely guarantee that everything thrown by the relevant try block is an Error? How can you guarantee that?

It seems like this should probably just be mixed?

This revision now requires changes to proceed.Apr 19 2022, 12:18 PM
lib/hooks/promote-sidebar.react.js
22 ↗(On Diff #11615)
const onError = e => {
  Alert.alert('Unknown error', 'Uhh... try again?', undefined, {
    cancelable: true,
  });
  throw e;
};

in D3755 we take the error message and throw it. But, I guess we could just throw it here in the hook and call it a day. Is that what you're getting at @ashoat?

  1. I don't. It could possibly be a DOMException (https://developer.mozilla.org/en-US/docs/Web/API/DOMException) if it's thrown in the browser, versus in native.

I think mixed would make sense if error needed to be used outside of the function. But, to your point in 1. I don't think we need to do that, or type it at all. Looking at the code base, catch(e) is never explicitly typed.

Image 2022-04-20 at 1.11.08 PM.jpg (1×1 px, 135 KB)

lib/hooks/promote-sidebar.react.js
22 ↗(On Diff #11615)

apologies, D3748 is the correct link to the diff referenced above.

This revision is now accepted and ready to land.Apr 20 2022, 8:04 PM