feat: avoid Google Translate plugin crashes#4203
Conversation
|
You can access the deployment of this PR at https://renku-ci-ui-4203.dev.renku.ch |
bb8ae89 to
7baccba
Compare
There was a problem hiding this comment.
The fix-method needs to run at the top of the client init script. At the moment the client entrypoint is hidden, so here is how to do it properly:
- Run
$ npx react-router revealinclient/:Entry file entry.client created at src/entry.client.tsx. Entry file src/entry.server.tsx already exists. - Open the new file
src/entry.client.tsxand edit it:
import { startTransition, StrictMode } from "react";
import { hydrateRoot } from "react-dom/client";
import { HydratedRouter } from "react-router/dom";
import { fixExternalDOMMutationsCrashes } from "./utils/...";
// Prevent crashes from browser extensions, e.g. Google Translate
fixExternalDOMMutationsCrashes();
startTransition(() => {
hydrateRoot(
document,
<StrictMode>
<HydratedRouter />
</StrictMode>,
);
});There was a problem hiding this comment.
We should keep the original implementation from React.
- Add a comment like this:
// Reference: https://github.com/facebook/react/issues/11538 // This fixes React crashing when the DOM is manipulated by extensions, e.g. Google Translate
- Name the method something like
fixExternalDOMMutationsCrashes() - Use the implementation from Make React resilient to DOM mutations from Google Translate facebook/react#11538 (comment). As far as we know, it's the best implementation of the fix (from the React team).
- Instead of logging to
consolewe can also consider sendingwarningorinfoto Sentry.
| import "./styles/renku_bootstrap.scss"; | ||
| import "./utils/bootstrap/bootstrap.client"; | ||
|
|
||
| removeChildGuard(); |
There was a problem hiding this comment.
This runs in the server too, so this is not the right place for this method.
| startTransition(() => { | ||
| // Prevent crashes from browser extensions like Google Translate | ||
| fixExternalDOMMutationsCrashes(); | ||
|
|
||
| hydrateRoot( | ||
| document, | ||
| <StrictMode> | ||
| <HydratedRouter /> | ||
| </StrictMode>, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
If the method in startTransition() is executed more than once, then maybe this is better:
| startTransition(() => { | |
| // Prevent crashes from browser extensions like Google Translate | |
| fixExternalDOMMutationsCrashes(); | |
| hydrateRoot( | |
| document, | |
| <StrictMode> | |
| <HydratedRouter /> | |
| </StrictMode>, | |
| ); | |
| }); | |
| // Prevent crashes from browser extensions like Google Translate | |
| fixExternalDOMMutationsCrashes(); | |
| startTransition(() => { | |
| hydrateRoot( | |
| document, | |
| <StrictMode> | |
| <HydratedRouter /> | |
| </StrictMode>, | |
| ); | |
| }); |
We can check if it's better outside of the startTransition().
There was a problem hiding this comment.
Sorry, I am not sure if it's best to call before startTransition or inside it 😅
There was a problem hiding this comment.
It should run only once, let's try to leave it outside 5509d5d
leafty
left a comment
There was a problem hiding this comment.
LGTM, one non-blocking comment below.
| @@ -0,0 +1,40 @@ | |||
| // Guards against React crashes when the DOM is manipulated by extensions like Google Translate | |||
| // ? Reference: https://github.com/facebook/react/issues/11538#issuecomment-417504600 | |||
| export function fixExternalDOMMutationsCrashes() { | |||
There was a problem hiding this comment.
Note: I have seen implementations use a variable on window to ensure this is executed only once, e.g.
if (typeof window === 'object' && window.__renku_appliedDOMFix == null) {
// ...body...
window.__renku_appliedDOMFix = true;
}There was a problem hiding this comment.
That's a good suggestion to prevent accidental double run. Worst case, it's an unnecessary safeguard
ffa7631
leafty
left a comment
There was a problem hiding this comment.
LGTM, tested on chrome: I crash in dev with translate and with this PR I can use translate and it seems to not crash.
|
Tearing down the temporary RenkuLab deplyoment for this PR. |
Avoid crashing the browser when Google Translate does its own thing with the DOM
/deploy renku=release-2.18.0
fix #4202