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: formally deprecate crashed and renderer-process-crashed events #40089

Merged
merged 1 commit into from Oct 10, 2023

Conversation

miniak
Copy link
Contributor

@miniak miniak commented Oct 4, 2023

Description of Change

Deprecated since #23096, #24410, #39494
Warnings fixed in #40090

Checklist

Release Notes

Notes: The renderer-process-crashed event on app and crashed event on WebContents and <webview> have been deprecated.

@miniak miniak self-assigned this Oct 4, 2023
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Oct 4, 2023
@miniak miniak force-pushed the miniak/deprecate-renderer-process-crashed branch from c5ee6c7 to 6cbbf84 Compare October 6, 2023 00:00
@miniak miniak marked this pull request as ready for review October 6, 2023 00:05
@miniak miniak requested a review from a team as a code owner October 6, 2023 00:05
@miniak miniak force-pushed the miniak/deprecate-renderer-process-crashed branch from 6cbbf84 to 62c436f Compare October 6, 2023 00:38
@miniak miniak added target/27-x-y PR should also be added to the "27-x-y" branch. and removed no-backport labels Oct 6, 2023
@electron-cation
Copy link

electron-cation bot commented Oct 6, 2023

🪦 Deprecation Checklist

🔥 New deprecations in this PR


@electron/wg-releases: Please confirm these deprecation changes conform to our deprecation policies listed in docs/breaking-changes.md, then check the applicable items in the checklist and remove any non-applicable items.

Copy link
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

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

The crashed and render-process-gone events are on WebContents, not BrowserWindow.

@dsanders11
Copy link
Member

Title of this PR should refer to renderer-process-crashed, not renderer-process-gone. If it's not too much effort, let's add tests which expect the deprecation messages (see the deprecation checklist from cation).

@miniak miniak force-pushed the miniak/deprecate-renderer-process-crashed branch from 62c436f to 6582736 Compare October 6, 2023 08:46
@miniak
Copy link
Contributor Author

miniak commented Oct 6, 2023

The crashed and render-process-gone events are on WebContents, not BrowserWindow.

@dsanders11 oops, I don't know how I missed that. Fixed now.

@miniak miniak changed the title chore: formally deprecate crashed and render-process-gone events chore: formally deprecate crashed and renderer-process-crashed events Oct 6, 2023
@miniak miniak force-pushed the miniak/deprecate-renderer-process-crashed branch from 6582736 to 0d3364a Compare October 6, 2023 08:48
@miniak
Copy link
Contributor Author

miniak commented Oct 6, 2023

#40090

@dsanders11 Sadly the tests cannot be added easily as the tests don't share the instance of lib/common/deprecate with Electron itself. It's an unfortunate consequence of #35311. I will try to do something about it in a separate PR.

@miniak miniak requested a review from dsanders11 October 6, 2023 09:20
@miniak
Copy link
Contributor Author

miniak commented Oct 6, 2023

@dsanders11 We need to do this #40124

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Oct 7, 2023
@miniak miniak removed the target/27-x-y PR should also be added to the "27-x-y" branch. label Oct 8, 2023
@miniak miniak force-pushed the miniak/deprecate-renderer-process-crashed branch from 0d3364a to 2992c94 Compare October 8, 2023 19:37
@miniak
Copy link
Contributor Author

miniak commented Oct 9, 2023

@dsanders11 the warnings are in main now

@dsanders11
Copy link
Member

@miniak, I'm not sure if #40124 was necessary here. I added spec/lib/deprecate-helpers.ts in #39405 and used that for expecting deprecation warnings in previous PRs. The implementation there works around the internal module issue by listening to console.warn and the warning event on process and expecting the output there, instead of relying on the details of the deprecation module.

@miniak
Copy link
Contributor Author

miniak commented Oct 9, 2023

@miniak, I'm not sure if #40124 was necessary here. I added spec/lib/deprecate-helpers.ts in #39405 and used that for expecting deprecation warnings in previous PRs. The implementation there works around the internal module issue by listening to console.warn and the warning event on process and expecting the output there, instead of relying on the details of the deprecation module.

@dsanders11 I completely missed that somehow. Fixing in #40146

@miniak miniak requested a review from zcbenz October 9, 2023 19:33
@jkleinsc jkleinsc merged commit 3e70692 into main Oct 10, 2023
16 checks passed
@jkleinsc jkleinsc deleted the miniak/deprecate-renderer-process-crashed branch October 10, 2023 23:49
@release-clerk
Copy link

release-clerk bot commented Oct 10, 2023

Release Notes Persisted

The renderer-process-crashed event on app and crashed event on WebContents and &lt;webview&gt; have been deprecated.

MrHuangJser pushed a commit to MrHuangJser/electron that referenced this pull request Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🗑 Complete
Development

Successfully merging this pull request may close these issues.

None yet

4 participants