Update eslint-config-expensify #6174
Conversation
|
Hey, I noticed you changed some webpack configuration files. This can break production builds. Did you remember to run a production build locally to verify they still work? |
marcaaron
changed the title
|
I might break this up a bit. 100+ file changes is obnoxious 😄 |
|
Cool yeah let me know, otherwise I can start reviewing - no problem. Either way, given the botify comment #6174 (comment), it sounds like it would be great to test the desktop app too? |
|
👍 I'll give it a whirl to make sure everything is still building. |
marcaaron
changed the title
|
Off hold 🎉 |
|
Updated |
src/libs/API.js
Outdated
| @@ -153,7 +159,8 @@ Network.registerResponseHandler((queuedRequest, response) => { | |||
| // There are some API requests that should not be retried when there is an auth failure like | |||
| // creating and deleting logins. In those cases, they should handle the original response instead | |||
| // of the new response created by handleExpiredAuthToken. | |||
| if (queuedRequest.data.doNotRetry || unableToReauthenticate) { | |||
| const shouldRetry = lodashGet(queuedRequest, 'data.shouldRetry', true); | |||
There was a problem hiding this comment.
Wouldn't it be safer to assume false as the default here?
There was a problem hiding this comment.
Most things should be retried and the exception is when things pass shouldRetry: false (previously this was doNotRetry). Thinking more on this.. maybe a better way to handle this would be to add shouldRetry: true as the default whenever a request is queued. That way we don't have to have to use lodashGet() in the two places.
There was a problem hiding this comment.
Ah ok good to know, I didn't know we considered it safe to consider that any command is retriable by default, because on other layers we consider it's not safe to retry unless the command is explicitly known to be idempotent.
src/libs/Network.js
Outdated
| * @param {String} [request.data.returnValueList] | ||
| * @return {Boolean} | ||
| */ | ||
| function canRetryRequest(request) { | ||
| const doNotRetry = lodashGet(request, 'data.doNotRetry', false); | ||
| const logParams = {command: request.command, doNotRetry, isQueuePaused}; | ||
| const shouldRetry = lodashGet(request, 'data.shouldRetry', true); |
There was a problem hiding this comment.
Same comment as above about default value.
src/libs/actions/App.js
Outdated
| @@ -40,6 +46,20 @@ function setLocale(locale) { | |||
| Onyx.merge(ONYXKEYS.NVP_PREFERRED_LOCALE, locale); | |||
| } | |||
|
|
|||
| function getLocaleAndUpdate() { | |||
There was a problem hiding this comment.
not a blocker - The function name sounds a little odd to me, like something was missing after "and update". Maybe loadLocale() would be an alternative.
There was a problem hiding this comment.
We are using get* or fetch* in a lot of methods so probably something like getLocale() is fine here?
App/src/libs/Navigation/AppNavigator/AuthScreens.js
Lines 155 to 157 in b770151
There was a problem hiding this comment.
Yes that sounds good to me!
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by @roryabraham in version: 1.1.14-5 🚀
|
|
🚀 Deployed to production by @AndrewGable in version: 1.1.15-15 🚀
|
| @@ -59,8 +79,14 @@ AppState.addEventListener('change', (nextAppState) => { | |||
| appState = nextAppState; | |||
| }); | |||
|
|
|||
| function triggerUpdateAvailable() { | |||
| Onyx.merge(ONYXKEYS.UPDATE_AVAILABLE, true); | |||
There was a problem hiding this comment.
Couldn't this be Onyx.set( since we're only storing a bool?
There was a problem hiding this comment.
Yeah, I think that's correct!
Details
Updates to the most recent version of
eslint-config-expensifywhich adds a ton of custom rules.Fixed Issues
$ #5995
Tests
No specific test steps
QA Steps
Nothing beyond normal regressions
Tested On
Screenshots
N/A