-
-
Notifications
You must be signed in to change notification settings - Fork 829
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
Patch: "Reloading the registration page should warn about data loss" #8377
Changes from 11 commits
ac39d61
3c47141
e37af06
2c9712b
b29c417
872d0e0
9fe77a8
8b0dca2
9b92c90
48636c6
584d54e
8ed065f
75c3627
71e9932
736eae1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -141,6 +141,22 @@ export default class Registration extends React.Component<IProps, IState> { | |
this.replaceClient(this.props.serverConfig); | ||
} | ||
|
||
componentDidUpdate(_, prevState: IState) { | ||
if (prevState.doingUIAuth !== this.state.doingUIAuth) { | ||
//triggers a confirmation dialog for data loss before page unloads/refreshes | ||
window.addEventListener("beforeunload", this.unloadCallback); | ||
} | ||
} | ||
|
||
componentWillUnmount() { | ||
window.removeEventListener("beforeunload", this.unloadCallback); | ||
} | ||
|
||
private unloadCallback = (event: BeforeUnloadEvent) => { | ||
event.preventDefault(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will always show the confirmation dialog
(Empirically only in Firefox & Safari) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's now working as it should. I have tested it in the three browsers. |
||
event.returnValue = ""; | ||
return ""; | ||
}; | ||
// TODO: [REACT-WARNING] Replace with appropriate lifecycle event | ||
// eslint-disable-next-line | ||
UNSAFE_componentWillReceiveProps(newProps) { | ||
|
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.
this could still register twice, once on doingUIAuth false->true and once back (and if it keeps toggling then it would keep going
One thing to note (which I think is stupid) about addEventListener, is it does not check if the listener is already bound & removeEventListener only removes one instance, not all instances. So this code really needs to be solid because otherwise you'll leave stray listeners hanging around which will keep the warning even much later into the app lifecycle
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.
I am not sure, i really understand your first statement clearly, but as for removeEventListener, when the component unmounts doesn't it remove all instances? since the listeners is only tied to one component, i guess it should be scoped within that component, this is the first time i am knowing about the one instance thing of listeners based on your text. So what do you suggest i do?
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 you call
addEventListener
5 times, 5 handlers will be registered (even if the same method is passed) -removeEventListener
only removes one of them, causing a memory leak by keeping a pointer to this component instance around.Maybe easier would be to register the handler always in componentDidMount, but do the if doingUIAuth check inside the handler
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 to ask again, i am finding it rather confusing that
addEventListener
will be registered as many times the method runs so that means theif
statement is not doing anything then cause it was supposed to only register the listener if there is a disparity between the prevState and the current State. Also if i placed the listener in componentDidMount, then that means , i 'll just putthis.state.doingUIAuth
as the if condition since there is no prevStateThere 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.
i realized
this.state.doingUIAuth
will always be false, so the listener will never be registered. if i placed it in thecomponentDidMount
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.
Right but that disparity could happen multiple times within a single mounting of this component, e.g if you start registration, do captcha, then finish captcha doingUIAuth would go false->true->false then unmount
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.
Precisely
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.
componentDidMount
only mounts once anddoingUIAuth
will be false, so there is no way it'll register the listener.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.
Yes, always register the handler, then inside
unloadCallback
you can checkdoingUIAuth
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.
Done