-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
feat: Add first-instance-ack
event to the app.requestSingleInstanceLock()
flow
#31460
feat: Add first-instance-ack
event to the app.requestSingleInstanceLock()
flow
#31460
Conversation
fa80609
to
c2218f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API looks good to me.
The tests are failing on Windows CI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An example should be provided showing how the ackCallback / first-instance-ack is used.
Also, this note from the PR should make its way into the documentation:
The new ackCallback parameter allows the first instance to pass back some data to the second instance. Currently, the limit is below kMaxMessageLength, or 32kB
Considering that the additionalData parameter in both the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rzhao271 can you describe a little more what you mean? All your tests are key-value pairs so it might help at baseline to add some tests which encompass the range of possible values. Also, to add to what @jkleinsc said above, the example provided in documentation should also include a concret sample for additionalData
(as noted in comment)
8f4063a
to
e5efbc2
Compare
57b250e
to
2883f59
Compare
I added some more tests in this PR, as well as some tests (that I'm hoping can be merged first) in #31661 to demonstrate the variety of data that can be passed. |
2883f59
to
6c457b1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the example of using ackCallback
isn't in the PR changes anymore? There should be a documented example in the docs of using ackCallback
9576d74
to
28ed73f
Compare
7c6b665
to
70a1013
Compare
Rebased, ready for review again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API LGTM
Release Notes Persisted
|
…eLock()` flow (electron#31460) * feat: Add onFirstInstanceAck event for requestSingleInstanceLock * Add tests * Apply patch fix * Add back missing docs * Rebase * Listen for exit earlier in test * Rebase
There seems to be a minor error in the documentation example code - https://www.electronjs.org/docs/latest/api/app#apprequestsingleinstancelockadditionaldata ackCallback is missing in return values of second-instance event which is throwing an error when trying to launch second instance of the electron app by clicking the desktop icon created by electron packager. This code with ackCallback as one of the args is working without any errors:
|
Description of Change
This PR adds a
first-instance-ack
event, as well as a parameterackCallback
to thesecond-instance
event.When the first instance handles the
second-instance
event, that means the second instance calledapp.requestSingleInstanceLock([obj])
, which sends data to the first instance. The newackCallback
parameter allows the first instance to pass back some data to the second instance. Currently, the limit is belowkMaxMessageLength
, or32kB
. End users can take advantage of this new API by first callingevent.preventDefault()
, and then calling the new callback parameter. They must also register afirst-instance-ack
event handler before therequestSingleInstanceLock
call is made to read the data.CC @deepak1556
Checklist
npm test
passesRelease Notes
Notes: Added
first-instance-ack
event to theapp.requestSingleInstanceLock()
flow, so that users can pass some data back from the second instance to the first instance.