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.

Sign up for GitHub

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

Jump to bottom

feat(web): add beforeunload event #14830

Merged
merged 14 commits into from
Jun 28, 2022
Merged

feat(web): add beforeunload event #14830

merged 14 commits into from
Jun 28, 2022

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jun 9, 2022

This commit adds the 'beforeunload' event.

@bartlomieju bartlomieju mentioned this pull request Jun 9, 2022
Merged
@bartlomieju
Copy link
Member

I changed this PR to require e.preventDefault() to be called to keep running the event loop. I'm not sure if that's desirable though - it poses a challenge how to polyfill process.on("beforeExit") in Node compat layer - we can check if there are active listeners and prevent the event, but I'm worried that it could cause infinite loops. @lucacasonato I'm interested in hearing your thoughts on the topic of this API.

@bartlomieju bartlomieju added this to the 1.24 milestone Jun 20, 2022
@bartlomieju bartlomieju marked this pull request as ready for review June 21, 2022 21:18
@bartlomieju
Copy link
Member

bartlomieju commented Jun 21, 2022 •
edited
Loading

It appears there are no WPT tests that we can enable. @lucacasonato please take a look, we should update compatibility tables after landing this PR (with a note that event.returnValue is ignored in Deno).

@cjihrig cjihrig merged commit 0f6a5c5 into denoland:main Jun 28, 2022
@cjihrig cjihrig deleted the unload branch June 28, 2022 14:49
cjihrig added a commit to cjihrig/browser-compat-data that referenced this pull request Jun 28, 2022
@cjihrig
Deno 1.24 supports the beforeunload event
375985e 
The Deno 1.24 release adds support for the beforeunload event.

Refs: denoland/deno#14830
@cjihrig cjihrig mentioned this pull request Jun 28, 2022
Merged
cjihrig added a commit to cjihrig/browser-compat-data that referenced this pull request Jun 28, 2022
@cjihrig
Deno 1.24 supports the beforeunload event
0c76183 
The Deno 1.24 release adds support for the beforeunload event.

Refs: denoland/deno#14830
@andreubotella
Copy link
Contributor

andreubotella commented Jul 10, 2022 •
edited
Loading

Is there any reason why this PR added op_event_loop_has_more_work and JsRuntime::event_loop_has_work, given that they're not used?

@nayeemrmn
Copy link
Collaborator

Is there any reason why this PR added op_event_loop_has_more_work and JsRuntime::event_loop_has_work, given that they're not used?

The former to use in denoland/std#2331, I guess the latter was supposed to be used in the op but was forgotten..

@andreubotella
Copy link
Contributor

In that case, given that that same code shows up three times (in JsRuntime::poll_event_loop plus in those two functions), I think event_loop_has_work should return a struct of booleans which can be used by both the event loop and this op.

andreubotella added a commit to andreubotella/deno that referenced this pull request Jul 11, 2022
@andreubotella
chore(core): Deduplicate code related to op_event_loop_has_more_work
62ad6da 
The `op_event_loop_has_more_work` op, introduced in denoland#14830, duplicates
code from `JsRuntime::poll_event_loop`. That PR also added the unused
method `JsRuntime::event_loop_has_work`, which is another duplication
of that same code, and which isn't used anywhere.

This change deduplicates this by renaming
`JsRuntime::event_loop_has_work` to `event_loop_pending_state`, and
making it the single place to determine what in the event loop is
pending. This result is then returned in a struct which is used both
in the event loop and in the `op_event_loop_has_more_work` op.
@andreubotella andreubotella mentioned this pull request Jul 11, 2022
Merged
bartlomieju pushed a commit that referenced this pull request Jul 11, 2022
@andreubotella
chore(core): Deduplicate code related to op_event_loop_has_more_work (
83c9714 
#15147)

The `op_event_loop_has_more_work` op, introduced in #14830, duplicates
code from `JsRuntime::poll_event_loop`. That PR also added the unused
method `JsRuntime::event_loop_has_work`, which is another duplication
of that same code, and which isn't used anywhere.

This change deduplicates this by renaming
`JsRuntime::event_loop_has_work` to `event_loop_pending_state`, and
making it the single place to determine what in the event loop is
pending. This result is then returned in a struct which is used both
in the event loop and in the `op_event_loop_has_more_work` op.
cjihrig added a commit to cjihrig/browser-compat-data that referenced this pull request Jul 21, 2022
@cjihrig
Deno 1.24 supports the beforeunload event
7a48abd 
The Deno 1.24 release adds support for the beforeunload event.

Refs: denoland/deno#14830
cjihrig added a commit to cjihrig/browser-compat-data that referenced this pull request Jul 21, 2022
@cjihrig
Deno 1.24 supports the beforeunload event
bcaaf6a 
The Deno 1.24 release adds support for the beforeunload event.

Refs: denoland/deno#14830
queengooborg pushed a commit to mdn/browser-compat-data that referenced this pull request Jul 21, 2022
@cjihrig
Deno 1.24 supports the beforeunload event (#16817)
82a90bf 
The Deno 1.24 release adds support for the beforeunload event.

Refs: denoland/deno#14830
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bartlomieju bartlomieju bartlomieju approved these changes

@lucacasonato lucacasonato Awaiting requested review from lucacasonato

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.24
Development

Successfully merging this pull request may close these issues.
4 participants
@cjihrig @bartlomieju @andreubotella @nayeemrmn