Skip to content
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

useOnBeforeUnload() on expensive dirty flag #842

Closed
bebbi opened this issue Dec 18, 2019 · 4 comments · Fixed by #867
Closed

useOnBeforeUnload() on expensive dirty flag #842

bebbi opened this issue Dec 18, 2019 · 4 comments · Fixed by #867
Labels
enhancement New feature or request help wanted Extra attention is needed released
Projects

Comments

@bebbi
Copy link
Contributor

bebbi commented Dec 18, 2019

Is your feature request related to a problem? Please describe.

I've considered the useOnBeforeUnload hook to warn if a doc has unsaved changes.
But it makes no sense because the dirty boolean flag is expensive to calculate (it's a deep comparison between serialized objects).

The way the hook currently works I have to recalculate the dirty flag much more often than necessary.

Describe the solution you'd like

I'd like to propose that

  1. the useOnBeforeUnload hook always registers a handler.
  2. I can optionally provide a dirty function instead of a boolean
  3. The handler, if called before window unload, calls dirty() if it's a function, but only at the actual time of unload where it's relevant.

That would reduce an expensive calculation to once when it's needed.
For simplicity, I think the approach can be the same for a boolean dirty flag since the handler won't take up noticeable resources.

Describe alternatives you've considered

I have considered flagging the isDirty on any doc change and avoiding re-calculation once it's true, but that doesn't make much sense because there's so many changes to the doc variable that are not relevant anymore after it's serialized (such as selection changes).

So it looks like I can either PR this for you or build my own hook.

@streamich
Copy link
Owner

streamich commented Dec 23, 2019

Would allowing enabled to be a function solve your performance concert?

useBeforeUnload(() => true, 'Are you sure?');

  • Allow enabled parameter to be a function
    • If enabled is a function it should execute only when component detects "unload" event
  • Still support old behaviour where, enabled is a boolean.

@streamich streamich added enhancement New feature or request help wanted Extra attention is needed labels Dec 23, 2019
@streamich streamich added this to To do in react-use via automation Dec 23, 2019
@bebbi
Copy link
Contributor Author

bebbi commented Dec 26, 2019

@streamich Yes, that looks good, I've enabled this in my own version.
The key was to supply a callback that compares refs, then it doesn't need to reload a new listener everytime the value changes. Let me know if you're interested in a PR.

@streamich
Copy link
Owner

@bebbi If you would like to create a PR, would be great!

react-use automation moved this from To do to Done Feb 15, 2020
streamich added a commit that referenced this issue Feb 15, 2020
feat(useBeforeUnload): allow passing a dirty function (#842)
streamich pushed a commit that referenced this issue Feb 15, 2020
# [13.25.0](v13.24.1...v13.25.0) (2020-02-15)

### Features

* **useBeforeUnload:** allow passing a dirty function ([#842](#842)) ([c4a14a4](c4a14a4))
@streamich
Copy link
Owner

🎉 This issue has been resolved in version 13.25.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed released
Projects
react-use
  
Done
Development

Successfully merging a pull request may close this issue.

2 participants