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
Details
Details
- Reviewers
ashoat ginsu - Commits
- rCOMM14b32af153a3: [native] VersionSupportedChecker component
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
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| 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 |
Comment Actions
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 |
Comment Actions
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? |
Comment Actions
This is a good start, but we should be doing the same stuff we do for the client_version_unsupported error from the keyserver:
- Your copy is different
- 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"
| 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 |
Comment Actions
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) | |
| 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 |
| 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 |
Comment Actions
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)