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

feat: Add first-instance-ack event to the app.requestSingleInstanceLock() flow #31460

Merged
merged 7 commits into from Jan 10, 2022

Conversation

rzhao271
Copy link
Contributor

@rzhao271 rzhao271 commented Oct 18, 2021

Description of Change

This PR adds a first-instance-ack event, as well as a parameter ackCallback to the second-instance event.

When the first instance handles the second-instance event, that means the second instance called app.requestSingleInstanceLock([obj]), which sends data to the first instance. 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. End users can take advantage of this new API by first calling event.preventDefault(), and then calling the new callback parameter. They must also register a first-instance-ack event handler before the requestSingleInstanceLock call is made to read the data.

CC @deepak1556

Checklist

Release Notes

Notes: Added first-instance-ack event to the app.requestSingleInstanceLock() flow, so that users can pass some data back from the second instance to the first instance.

@rzhao271 rzhao271 requested a review from a team as a code owner October 18, 2021 19:43
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Oct 18, 2021
@zcbenz zcbenz added api-review/requested 🗳 semver/minor backwards-compatible functionality labels Oct 21, 2021
docs/api/app.md Outdated Show resolved Hide resolved
docs/api/app.md Outdated Show resolved Hide resolved
patches/chromium/.patches Outdated Show resolved Hide resolved
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 looks good to me.

@zcbenz
Copy link
Member

zcbenz commented Oct 25, 2021

The tests are failing on Windows CI.

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.

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

@rzhao271
Copy link
Contributor Author

Considering that the additionalData parameter in both the second-instance event and first-instance-ack event are base::Value objects, I'm wondering how I should describe those objects in the API? I currently wrote that they are JSON objects, but I feel that might lead users to think that they can only pass in key-value pairs, when they can pass in numbers and strings as well.

Copy link
Member

@codebytere codebytere left a 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)

docs/api/app.md Outdated Show resolved Hide resolved
spec-main/api-app-spec.ts Outdated Show resolved Hide resolved
@rzhao271
Copy link
Contributor Author

rzhao271 commented Nov 2, 2021

@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.

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.

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.

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

@rzhao271 rzhao271 force-pushed the rzhao271/single-instance-ack branch from 7c6b665 to 70a1013 Compare January 7, 2022 20:15
@rzhao271
Copy link
Contributor Author

rzhao271 commented Jan 8, 2022

Rebased, ready for review again

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

@release-clerk
Copy link

release-clerk bot commented Jan 10, 2022

Release Notes Persisted

Added first-instance-ack event to the app.requestSingleInstanceLock() flow, so that users can pass some data back from the second instance to the first instance.

t57ser pushed a commit to t57ser/electron that referenced this pull request Jan 25, 2022
…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
@mallamsripooja
Copy link

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:

... 

if (!gotTheLock) {
  app.quit()
} else {
  app.on('second-instance', (event, commandLine, workingDirectory, additionalData, ackCallback) => {

...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants