-
-
Notifications
You must be signed in to change notification settings - Fork 177
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade React Router to 6.14.2 and use upstream usePrompt #4505
Conversation
@@ -30,7 +30,7 @@ class SearchBoxStore extends Reflux.Store { | |||
}; | |||
|
|||
init() { | |||
history.listen(this.onRouteChange.bind(this)); | |||
setTimeout(() => router.subscribe(this.onRouteChange.bind(this))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we end up with these ugly setTimeout
s (which are fine hacks IMHO), it would be good to leave a comment somewhere. Adding same comment to each setTimeout
would ba an overkill, but perhaps a comment in legacy.tsx
would suffice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I'll comment the router variable.
* Used to force refresh form sub routes. It's some kine of a weird | ||
* way of introducing a loading screen during sub route refresh. | ||
* See: https://github.com/kobotoolbox/kpi/issues/3925 | ||
**/} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned on Zulip, we can drop this 🎉
I've looked at that
/forms/<uid>/reset
route. It is being used only intriggerRefresh()
function fromformViewTabs.es6
. The idea was that when user clicks a menu item, e.g. "Table" from "DATA", the route would reload itself. This is interesting, but since I broke it (of course it was me) over 2 years ago (see f14005c#diff-cd8026c65e0451326d9e9eb4de92a70018810bf708c2e70affcdab16773a93b1L251) it stopped working at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, I removed all code references. I should have made it more clear, did you notice that FORM_REPORT_OLD is also gone? It looked like that path never worked, typescript pointed this out. Can you confirm that we don't use that non-existent route? It's possible we lied to TypeScript and do use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FORM_KOBOCAT (/forms/:uid/settings/kobocat
) should be removed while we're here, it's superceded by the feature at /forms/:uid/settings/media
I'm feeling a lot more confident with better types now - but we still should manually test this more before merging. |
…using the package-lock.json from beta and re-running [email protected] install
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we got to talk today, I'll try changing some of the unhandled router?.
to router!.
to surface any issues with the router initialization.
I also want to subject it to more manual QA before merging.
It will break some work-in-progress branches like #4535.
If any of these functions are called while router is still null, that means our setTimeout(()=>…) dependency injection didn't work. Let's make that show up as an error in the browser console, rather than silently failing to navigate.
I'd like to make sure whenever we update this library, we do it as a focused and intentional endeavor, where someone gets to read the patch notes, and decide how much manual QA is needed. Patch notes for react-router: - https://github.com/remix-run/react-router/releases That might be true even if we weren't using `unstable_` API's. An alternative would be to use `~6.14.1` to stay on `6.14.x`, allowing some `npm` commands to sometimes update patch versions if they exist. --- Package-lock.json already specifies 6.14.1 prior to this commit, because it updated while I was resolving the merge conflict (6.14.1 qualifies as ^6.13.0).
Language selector had an extra box outline introduced by #4207 and the click height was short. (UI fix not related to current PR, this came up while testing the form processing route.)
Superceded by /forms/:uid/settings/media
formatting-only commit
- Manually hoist variables to make scope clearer and use 'let' - Carefully replace 8 loose comparisons with strict ones (===, !==) - https://dorey.github.io/JavaScript-Equality-Table/ was useful here
It seems asset.content.translations can be undefined if you have a form with 0 questions, which is pretty uncommon but it leads to a full-page crash here. Fix the same crash when it appears in the data table settings menu, and on the data reports page.
…as it still has some impact on subroute behavior.
@@ -27,46 +27,51 @@ import { | |||
QUERY_LIMIT_DEFAULT, | |||
} from '../constants'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry in advance for map.es6
. Best to review these commits separately.
- 🤖 Prettier and ESLint autofixes here
- ✋ Manual ESLint fixes here
- Manually hoist variables to make scope clearer and use 'let'
- Carefully replace 8 loose comparisons with strict ones (
===
,!==
)- https://dorey.github.io/JavaScript-Equality-Table/ was useful here
- ✋ Fix map screen crash here
- This turned out not to be a regression, but part of a series of crashes that would only happen in obscure cases.
6.136.14.2Reviewer Notes