Fix: Cannot create BrowserWindow before app is ready #79
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
👋🏻 @katinthehatsite. I coincidently encountered the test failures blocking this PR. I addressed them in #80 if you have time to review. 🙇🏻 Once that merges, updating this branch with the latest |
Contributor
Author
|
Hye @dcalhoun - thanks for the fix! I approved the PR you mentioned 👍 |
…or-create-browser-window
This reverts commit 49eb9fa.
Member
|
I updated trunk and that fixes the unit tests 👌 . I also forgot to change the branch, and continued working in other issue. I pushed a commit and then reverted it 😅 , sorry for the noise. |
wojtekn
approved these changes
May 6, 2024
Contributor
wojtekn
left a comment
wojtekn
left a comment
There was a problem hiding this comment.
It works fine, and the code change looks clear. Thanks for the fix.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related to https://github.com/Automattic/dotcom-forge/issues/6878
Proposed Changes
This PR adds additional
app.isReady()check toapp.on( 'activate')to ensure that the app is ready before it attempts to create a browser window.Testing Instructions
nvm use && npm install && npm startto start the appcmd + Won macOS to close the app window but keep the app runningNotes
I spent some time trying to trigger the error manually but with no luck. It seems that the steps that a user might take to trigger this error would be (since
activateevent is specifically designed for macOS, it would need to be tested on Mac):activateevent starting early before the app is ready by adding something likeapp.emit( 'activate' )early after app initialization but that also did not trigger the error on my end;app.isReady()would need to betruefirst before the BrowserWindow is created. It seems that theactivateevent can possible fire ( https://github.com/Automattic/dotcom-forge/issues/6878#issuecomment-2087334306 ) before thereadyevent so it makes sense to add this guard in that case.If you have any good suggestions on how to test this, let me know!
Pre-merge Checklist