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

chore: remove deprecated 'new-window' event #34526

Merged
merged 1 commit into from Aug 9, 2022

Conversation

miniak
Copy link
Contributor

@miniak miniak commented Jun 13, 2022

Description of Change

It has been deprecated since Electron 13. Warning was added in Electron 20 (#34528).

Checklist

Release Notes

Notes: The deprecated new-window event has been removed.

@miniak miniak requested a review from a team as a code owner June 13, 2022 17:29
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 13, 2022
@miniak miniak self-assigned this Jun 13, 2022
@miniak miniak added the semver/major incompatible API changes label Jun 13, 2022
Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm worried about removing this without any machinery to explode when people try to do things the old way. we used to recommend new-window listeners for security reasons, and i bet a lot of apps still use it. removing it without an error/warning seems dangerous.

i suggest a 2-version deprecate/delete cycle:

  1. in version X, add a warning when an app registers a new-window listener.
  2. in version X+1, remove the event and throw an error when the app registers a new-window listener.

@miniak
Copy link
Contributor Author

miniak commented Jun 13, 2022

@nornagon can we still add the warning in Electron 20 to be able to land the removal in Electron 21?

@nornagon
Copy link
Member

@miniak I'm not necessarily against that, but it's been deprecated for over a year now so I'm not sure waiting one more version to delete it is that bad? Is there any rush?

@miniak
Copy link
Contributor Author

miniak commented Jun 13, 2022

@nornagon no rush actually. I can add the warning only to main and wait with this one. What do others in the @electron/wg-api think?

@samuelmaddock
Copy link
Member

Adding a deprecation log and waiting seems like the correct approach. We may not be considering a certain use case and users can voice their concern prior to removal if they notice the deprecation.

If we can afford it, maybe waiting 2 versions would be worthwhile since not everyone upgrades to the latest version.

@miniak
Copy link
Contributor Author

miniak commented Jun 13, 2022

@samuelmaddock the question was mainly whether to do warning in E21 and then remove in E22, or already warn in E20 and remove in E21. We can also warn in E20 and remove in E22.

@samuelmaddock
Copy link
Member

We can also warn in E20 and remove in E22.

This sounds best to me to give folks upgrading ample time.

@miniak miniak force-pushed the miniak/remove-new-window-event branch 4 times, most recently from aa90087 to eb0691b Compare June 16, 2022 01:19
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 20, 2022
@miniak miniak force-pushed the miniak/remove-new-window-event branch from eb0691b to a2b030c Compare June 21, 2022 13:07
@miniak miniak force-pushed the miniak/remove-new-window-event branch 3 times, most recently from 9aee851 to 8247a3e Compare August 8, 2022 03:19
@miniak miniak added the wip ⚒ label Aug 8, 2022
@miniak miniak force-pushed the miniak/remove-new-window-event branch from 8247a3e to c61a678 Compare August 8, 2022 03:36
@miniak miniak removed the wip ⚒ label Aug 8, 2022
@miniak miniak requested a review from nornagon August 8, 2022 03:38
Copy link
Contributor

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API LGTM

@nornagon
Copy link
Member

nornagon commented Aug 8, 2022

API LGTM

Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API LGTM

@jkleinsc jkleinsc merged commit 8646bf8 into main Aug 9, 2022
@jkleinsc jkleinsc deleted the miniak/remove-new-window-event branch August 9, 2022 21:57
@release-clerk
Copy link

release-clerk bot commented Aug 9, 2022

Release Notes Persisted

The deprecated new-window event has been removed.

schetle pushed a commit to schetle/electron that referenced this pull request Nov 3, 2022
Stanzilla added a commit to WeakAuras/WeakAuras-Companion that referenced this pull request Jan 13, 2023
This was removed from Electron recently: electron/electron#34526
Stanzilla added a commit to WeakAuras/WeakAuras-Companion that referenced this pull request Jan 14, 2023
This was removed from Electron recently: electron/electron#34526
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants