Page MenuHomePhabricator

[native] VersionSupportedChecker component
ClosedPublic

Authored by varun on Nov 9 2023, 3:48 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Oct 2, 1:26 PM
Unknown Object (File)
Sun, Sep 22, 9:02 PM
Unknown Object (File)
Sat, Sep 7, 12:46 PM
Unknown Object (File)
Aug 29 2024, 5:17 PM
Unknown Object (File)
Aug 1 2024, 5:49 AM
Unknown Object (File)
Aug 1 2024, 5:49 AM
Unknown Object (File)
Aug 1 2024, 5:49 AM
Unknown Object (File)
Aug 1 2024, 5:49 AM
Subscribers

Details

Summary

this component calls the identity service and displays an alert if the client code version is not supported by the identity service.

Depends on D9813

Test Plan

built the native app with an unsupported code version and saw the alert. built with the actual code version and no alert was displayed.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

varun requested review of this revision.Nov 9 2023, 4:27 PM
native/components/version-supported.react.js
8 ↗(On Diff #33042)

if i understood correctly, the difference between function declaration syntax and arrow function syntax is how this is handled, which is not an issue here since i'm not using this at all. i can change this to the traditional syntax, though, if preferred

9 ↗(On Diff #33042)

i don't think i need useCallback here since the useEffect hook with an empty dependency array ([]) ensures that the checkVersionSupport function is called only once when the component mounts

ashoat requested changes to this revision.Nov 9 2023, 6:25 PM

This technically works, but something is going on where an ESLint rule isn't firing... you COULD ignore that and the component will work fine, but it's best to follow the conventions to avoid potential future pitfalls

native/components/version-supported.react.js
8 ↗(On Diff #33042)

Please use the traditional syntax – best to match established conventions, and this might be the reason the ESLint rule below didn't fire

9 ↗(On Diff #33042)

It's best to use useCallback still. In general you should just always use it, it helps you avoid mistakes, and will let you skip out on suppressing the ESLint warning below

24 ↗(On Diff #33042)

checkVersionSupport is missing in the dep array, surprised ESLint didn't catch this

It's best to add it there, at which point you'll also convert checkVersionSupport to a React.useCallback

This revision now requires changes to proceed.Nov 9 2023, 6:25 PM

address feedback

native/components/version-supported.react.js
24 ↗(On Diff #33042)

yeah that was intentional. empty array means this effect will only run once, like componentDidMount, right?

This is a good start, but we should be doing the same stuff we do for the client_version_unsupported error from the keyserver:

  1. Your copy is different
  2. You don't appear to be logging out the user

At the least please make sure you use the exact same copy. Don't copy-paste; you should factor it out.

Logging out the user can be handled in a separate diff if you'd prefer, just let me know.

yeah that was intentional. empty array means this effect will only run once, like componentDidMount, right?

Yes, but the code you have now does the same thing and doesn't break any of the "Rules of Hooks"

ashoat requested changes to this revision.Nov 10 2023, 12:59 PM
This revision now requires changes to proceed.Nov 10 2023, 12:59 PM

We should also make sure we don't log out if we're already logged out

remove unnecessary dependency

native/components/version-supported.react.js
28 ↗(On Diff #33395)

next week when i work on identity service log out on native, i'll update this code to first log out of identity service and then the keyserver

ashoat requested changes to this revision.Nov 19 2023, 11:31 AM
ashoat added a subscriber: inka.

One last round here, this looks almost ready

native/components/version-supported.react.js
4 ↗(On Diff #33395)

You'll actually need to use the useSelector in native/redux/redux-utils.js. That one is typed using our specific state types, and following the upcoming Flow upgrade we'll see type errors without it

21 ↗(On Diff #33395)

cc @inka @tomek, another one of these getting added

51 ↗(On Diff #33395)

Historically React Native has not handled console.error and console.warn very well: https://github.com/facebook/react-native/issues?q=is%3Aissue+in%3Atitle+%22console.warn%22+is%3Aclosed, https://github.com/facebook/react-native/issues?q=is%3Aissue+in%3Atitle+%22console.error%22+is%3Aclosed+

Might be worth testing to make sure this doesn't break anything. You could also consider just using console.log

native/utils/alert-messages.js
6–7 ↗(On Diff #33395)

Best to make these read-only

10 ↗(On Diff #33395)

Thank you for factoring this out!! Next time would be great to do it as a separate, preceding diff – but no need to change now

10 ↗(On Diff #33395)

Does this really need to be a function? Looks like it's always the same result... could just export a constant AlertDetails instead

This revision now requires changes to proceed.Nov 19 2023, 11:31 AM
varun added inline comments.
native/components/version-supported.react.js
51 ↗(On Diff #33395)

tested this by not running my local identity service, triggering the error log. app continued to work fine

varun marked an inline comment as done.

address latest feedback

tested this by not running my local identity service, triggering the error log. app continued to work fine

As a last step, can you make sure you try the console.error on the other platform? Eg. if you tried on iOS initially, can you try on Android as well? (As an alternative, you can simply replace the console.error with console.log)

This revision is now accepted and ready to land.Nov 21 2023, 3:39 AM

tested this by not running my local identity service, triggering the error log. app continued to work fine

As a last step, can you make sure you try the console.error on the other platform? Eg. if you tried on iOS initially, can you try on Android as well? (As an alternative, you can simply replace the console.error with console.log)

i'll just change it to console.log

This revision was automatically updated to reflect the committed changes.