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

Is window.onbeforeunload impelementation acceptable? #1056

Open
andrei0x309 opened this issue May 7, 2021 · 4 comments
Open

Is window.onbeforeunload impelementation acceptable? #1056

andrei0x309 opened this issue May 7, 2021 · 4 comments

Comments

@andrei0x309
Copy link
Contributor

andrei0x309 commented May 7, 2021

I made a branch ( https://github.com/andrei0x309/worker-dom/tree/window-on-before-unload ) with window.onbeforeunload implementation and demo, I think it's useful, from what I see onbeforeunload has 3 useful applications:

  1. show a loader when navigating to simulate a SPA( works well on modern browsers because they only render elements when they are loaded which means no white pages like in the old days)
  2. alerting the user of unsaved data
  3. analytics

The only problem I see is that some developers might abuse this feature( since it transfers a function on the main thread), so that's why I am asking if such a feature is acceptable to know if I should write some tests and submit a PR.

Thanks.

@samouri
Copy link
Member

samouri commented May 11, 2021

Hi @andrei0x309,

This is a very interesting proposal! Also your implementation looks pretty good.

Unfortunately, we do not want to allow for a fully dynamic Function due to the security concerns it comes along with.

@andrei0x309
Copy link
Contributor Author

Hi @samouri.

Thanks for your feedback, I'll see if I can do it another way.

Maybe you can leave this issue open for a few weeks to see if I can do something.

@andrei0x309
Copy link
Contributor Author

andrei0x309 commented May 12, 2021

@samouri

I changed the code now the onbeforeunload user-provided function is executed on the worker thread instead of the main thread, and there's no Function constructor invoked either, the caveat is the worker thread can't directly prevent the navigation so I made a custom API window.onbeforeunloadPreventNavigation( default false, to set if the function should prevent navigation) to support also that user case(personally I don't need to prevent navigation but there may be devs that need that.)

@samouri
Copy link
Member

samouri commented May 14, 2021

@andrei0x309, I'm out of office for the next 3 weeks. I'll be able to review your updates when I'm back

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants