New makeSingleInstance API #12782

MarshallOfSound · 2018-05-02T04:28:21Z

New usage is below

const {app} = require('electron')
let myWindow = null

const gotTheLock = app.requestSingleInstanceLock()

app.on('second-instance', (commandLine, workingDirectory) => {
  // Someone tried to run a second instance, we should focus our window.
  if (myWindow) {
    if (myWindow.isMinimized()) myWindow.restore()
    myWindow.focus()
  }
})

if (!gotTheLock) {
  return app.quit()
}

// Create myWindow, load the rest of the app, etc...
app.on('ready', () => {
})

IMPORTANT: This is not a breaking change, the old syntax still works but is deprecated and should be removed in 4.0.0

* new API `app.isPrimaryInstance()` * new API `app.isSingleInstance()` * new event `app.on('second-instance')` * deprecated old syntax `app.makeSingleInstance(cb)` * deprecated old syntax of `app.makeSingleInstance() --> bool` in favor of `app.isPrimaryInstance()`

MarshallOfSound · 2018-05-02T04:31:57Z

Although the new API requires two API calls instead of one to make your app single instance and then figure out if you should quit or not I think it is a much better API as it is way clearer what is going on for devs 👍

ckerr

Implementation comments inline.

Coding aside, I do wonder if this public API could be simplified before merging. We've expanded from one method to three, which must be called in a specific order or exceptions get thrown, and which in practice wouldn't be called without the other being called too.

I agree with the idea of switching from a callback to a signal, but maybe this could be done with a simpler API by just removing the callback arg from app.makeSingleInstance()?

ckerr · 2018-05-02T04:41:53Z

atom/browser/api/atom_api_app.cc

+bool App::OnSecondInstance(const base::CommandLine::StringVector& cmd,
+                           const base::FilePath& cwd) {
+  Emit("second-instance", cmd, cwd);
+  // This value is used literally no where, but we need it for Reasons(tm)


This is the kind of comment that makes kittens cry.

What would be good is adding something to planned-breaking-changes.md that discusses the future change (and links to the issues), and having these changes refer to planned-breaking-changes.md

To be fair this comment and the return value still being there is me not being bothered to refactor a little bit more 😆 I'll clear this out and make it return void 👍

ckerr · 2018-05-02T04:45:03Z

atom/browser/api/atom_api_app.cc

+  return false;
+}
+
+bool App::IsPrimaryInstance(mate::Arguments* args) {


method should be const

args should be const (we only use args->ThrowError(), which is also const)

ckerr · 2018-05-02T04:47:35Z

atom/browser/api/atom_api_app.cc

+  if (single_instance_state_ == SingleInstanceState::UNINITIALIZED) {
+    args->ThrowError(
+        "You can not check if you are the primary instance without calling "
+        "makeSingleInstance");


(bikeshed) "Must call makeSingleInstance() before IsPrimaryInstance()"

ckerr · 2018-05-02T04:49:53Z

atom/browser/api/atom_api_app.cc

  if (process_singleton_)
+    return true;
+  return false;


did return process_singleton_; give warnings?

ckerr · 2018-05-02T04:50:48Z

atom/browser/api/atom_api_app.cc

+  return single_instance_state_ == SingleInstanceState::PRIMARY;
+}
+
+bool App::IsSingleInstance() {


Method should be const

ckerr · 2018-05-02T04:55:04Z

atom/browser/api/atom_api_app.h

+  UNINITIALIZED,
+  PRIMARY,
+  SECONDARY,
+};


Consider making this an enum class

ckerr · 2018-05-02T05:11:04Z

docs/api/app.md

-command line arguments, and `workingDirectory` is its current working directory. Usually
-applications respond to this by making their primary window focused and
-non-minimized.
+### `app.isSingleInstance()`


Does this need to be public API? It's not in your new use example in the PR

Doesn't need to be, but it's helpful for modules that need to know whether or not they are in a single instance app

OK now we're getting somewhere, maybe there is a use case for these that I'm not seeing.

When would this method be useful?

ckerr · 2018-05-02T05:13:24Z

docs/api/app.md

 process has sent its parameters to another instance, and you should immediately
 quit.

+This method will throw an error if you call it without first calling
+`app.makeSingleInstance()`.


It never makes sense to call makeSingleInstance() without also calling this

It never makes sense to call this without calling makeSingleInstance() first

Why not make these a single API call as before? I don't see how that goes against the goal of replacing a callback with a signal?

Because makeSingleInstance returning a boolean that indicates whether it is the second instance doesn't semantically make a whole lot of sense IMO. This might be API preference creeping in but I 100% prefer a slightly more verbose self-explanatory API than an API that doesn't make sense without reading the docs to figure out what the return value of makeSingleInstance means.

Like I've seen this before:

if (app.makeSingleInstance(cb)) { return app.quit(); }

Which although is API correct, makes no sense to anyone reading that. Especially when most "action" api's that Electron exposes, the boolean value indicates success.

We're in agreement the current API is unintuitive -- the proposed API is more readable and has return values that make sense.

However, I'm not sure how useful these new methods are. I don't see a use case for IsSingleInstance() and it doesn't seem that makeSingleInstance() and IsPrimaryInstance() make use except when called as a pair, suggesting that they could be folded into a single function.

Which comes back to the current API: putting aside the unintuitive return value and slightly opaque name for the moment, it does have strengths in that it's impossible to call the pieces out of order, to miss a call, or to forget to register for a signal. It's very difficult to get wrong.

So what if we preserved that while fixing the name and return value? Something like:

if (!app.registerSecondInstanceHandler(cb)) { app.quit(); }

This seems pretty readable to me.

MarshallOfSound · 2018-05-02T06:03:46Z

We've expanded from one method to three

nit: Only 2, the third method is not required, it's just there as a helper for modules to use.

Explained my reasoning behind it here: #12782 (comment)

ckerr · 2018-05-02T23:13:13Z

atom/browser/api/atom_api_app.h

+  bool OnSecondInstance(const base::CommandLine::StringVector& cmd,
+                        const base::FilePath& cwd);
+  bool IsPrimaryInstance(mate::Arguments* args);
+  bool IsSingleInstance();


If we do go with this three-method API ... these two functions have names which are confusingly similar to each other. Since we're trying to make this readable without reaching for the docs, is there a clearer way to name these?

…opriate deprecation comments

ckerr

Changes requested, but I like where this is heading and appreciate the collaboration. 👍

ckerr · 2018-05-03T04:38:40Z

lib/browser/api/app.js

+  }
+  return !app.requestSingleInstanceLock()
+}
+


Needs a deprecate mesage for app.releaseSingleInstance() too

ckerr · 2018-05-03T04:40:38Z

atom/browser/api/atom_api_app.cc

-    const ProcessSingleton::NotificationCallback& callback,
+    const base::Callback<
+        void(const base::CommandLine::StringVector& command_line,
+             const base::FilePath& current_directory)> callback,


- current_directory)> callback, + current_directory)>& callback,

I feel bad for losing that... 😆

ckerr · 2018-05-03T04:44:56Z

atom/browser/api/atom_api_app.cc

+  Emit("second-instance", cmd, cwd);
+}
+
+bool App::HasSingleInstanceLock() const {


not to beat a dead horse, but what is the use case for this method being public API? 😃

I'm not completely opposed to this; more like I need help understanding who would use this

Example: A helper module for handling opening files in an Electron app, will need to know whether to expect second-instance events or whether to handle multiple instances 👍

Basically helpful for third party modules that devs might use, not first party code inside apps.

ckerr · 2018-05-03T04:48:04Z

docs/api/app.md

-const isSecondInstance = app.makeSingleInstance((commandLine, workingDirectory) => {
+const gotTheLock = app.requestSingleInstanceLock()
+
+app.on('second-instance', (commandLine, workingDirectory) => {
  // Someone tried to run a second instance, we should focus our window.
  if (myWindow) {
    if (myWindow.isMinimized()) myWindow.restore()
    myWindow.focus()


Hey, not a blocker to this review (esp since it's not new to this diff) but what is the difference between win.restore() and win.show()?

It looks like this block could be replaced with if (myWindow) myWindow.show() to restore + focus but it's not completely clear to me from the documentation, so I thought I'd ask...

ckerr · 2018-05-03T04:49:44Z

docs/api/app.md

  app.quit()
+} else {
+  // Create myWindow, load the rest of the app, etc...


Non-primary apps shouldn't register for 'second-instance' events, so the app.on('second-instance' should probably go here. Devs are going to copy-paste this, so let's make it 💯

ckerr · 2018-05-03T04:51:56Z

lib/browser/api/app.js

@@ -109,6 +109,15 @@ for (let name of events) {
  })
 }

+// TODO(MarshallOfSound): Remove in 4.0


Should be added to docs/tutorial/planned-breaking-changes.md

ckerr · 2018-05-03T04:54:38Z

docs/api/app.md

-application and your app should continue loading. And returns `true` if your
-process has sent its parameters to another instance, and you should immediately
-quit.
+I.e. This methods returns `true` if your process is the primary instance of your


s/methods/method/

ckerr · 2018-05-03T04:54:59Z

docs/api/app.md

-quit.
+I.e. This methods returns `true` if your process is the primary instance of your
+application and your app should continue loading.  It returns `false` if your
+process should immediately quit as it has sent it's parameters to another


s/it's/its/

…nges

ckerr

LGTM. 👍

Since this deprecates existing API I'd like to get a second signoff on this before merging -- but FWIW this addresses all the issues I've commented on over the last day. 💯

codebytere

Per readthrough and @ckerr's comments, i think this is ready for prime-time 🚀

See electron/electron#12782

shesek · 2018-07-10T02:15:24Z

It looks like the event listener is called with (event, args, cwd), not with the documented (args, cwd). Which is the correct behavior?

schontz · 2018-10-15T21:09:26Z

I assume this is available as of 3.0.0? The release page doesn't exactly make it clear: https://electronjs.org/releases.

lsegal · 2018-10-30T00:20:15Z

I just want to point out that:

IMPORTANT: This is not a breaking change, the old syntax still works but is deprecated and should be removed in 4.0.0

Is the definition of a breaking change.

Is there a technical reason to break such a heavily used API in 4.0.0? All I'm seeing in the explanation is that this is "better", but I don't think it was clearly communicated as to why.

schontz · 2018-10-30T00:29:52Z

Deprecated is still not quite breaking. It will break when removed. Thankfully, we should have plenty of time to fix before 4.0.0.

MarshallOfSound · 2018-10-30T00:30:08Z

@lsegal This PR in itself was not a breaking change, although it was still released in a semver/major bump (3.0) with deprecation warnings as per our deprecation policy. We then removed the old implementation in another semver/major bump (4.0), this bit was a breaking change.

Is there a technical reason to break such a heavily used API in 4.0.0?

It makes the API easier to use by both us internally and third party modules. E.g. Modules don't have to call makeSingleInstance in order to get information about other instances launching. They can now listen to the second-instance event. It removes a lot of boilerplate that most apps have to implement in order to use makeSingleInstance to do anything even remotely complex with the second instances args.

It's a better API and we gave two semver/major cycles with a deprecation warning to move over to the new API (which isn't much of a refactor)

lsegal · 2018-10-30T00:48:02Z

It makes the API easier to use by both us internally and third party modules.

This, to me, isn't a technical reason to break the API, this is a technical reason to introduce a new API. You're causing churn on downstream users because of a use case that your downstream users don't even need. I am reading this as: makeSingleInstance() worked fine but we wanted more functionality, and focusing in on the "worked fine" part.

It's a better API and we gave two semver/major cycles with a deprecation warning to move over to the new API

This may not be the right place for this discussion and I'm fine with redirecting, but this is not a good policy. Realize that Electron 2.x is less than 6 months old. Six months. In other words, there is a large class of your users who have only just moved to 2.x. Expecting users to see the notices in 3.x is really not a viable option, since many legacy users (the ones who are actually using this API) probably haven't even moved to 3.x yet. I'm currently upgrading an app from 2.x.

Electron should have a more robust deprecation policy based on time, not version numbers, especially if Electron is going to be bumping major versions every time you decide Electron doesn't like the name of an API. I'm already seeing planned breaking changes for 5.x in one of the docs, and 5.x doesn't even exist yet.

... (which isn't much of a refactor)

Please empathize with your users. One line of code may not seem like a large refactor to you, but every time you change a function because you don't like a name, you're adding one more line of code that downstream users have to deal with. The fix is obvious to you, but each of these fixes has to be researched, understood, tested, and reviewed on downstream codebases. For any user who is still on Electron 1.x (of which there are probably many), they are dealing with 100 other refactors, and now 101. Each of these adds up to create a ton of friction to upgrading. I've already held off 3.x upgrades because of churn, and now it's getting worse.

t;ldr I personally don't see why makeSingleInstance() isn't just turned into an alias with something reasonable like a 2 year deprecation period. Given that it's almost the exact same API, there's almost no maintenance burden to Electron in keeping it in your codebase. The "small refactor" is something Electron can own instead of pushing it to every single downstream legacy user.

MarshallOfSound · 2018-10-30T01:53:47Z

This may not be the right place for this discussion and I'm fine with redirecting, but this is not a good policy.

We have a strictly documented Versioning Policy that was written with consensus of the entire maintainer community with feedback from lots of the top Electron apps. We didn't just make it up one day and it is a living document being improved on.

Please empathize with your users. One line of code may not seem like a large refactor to you, but every time you change a function because you don't like a name, you're adding one more line of code that downstream users have to deal with.

I 💯% empathize with this, but this change wasn't just "this name isn't that good", it's "this function was not very usable in its current state for more complex use cases". If tracking down these kinds of changes are proving tricky I'd suggest considering a type system in your codebase (for example Typescript which we output type definitions for and therefore would catch any and all API changes for you).

especially if Electron is going to be bumping major versions every time you decide Electron doesn't like the name of an API

We definitely do not bump major just because we want to rename an API, we have very strictly defined requirements for a what constitutes a major bump and although breaking API changes can only go out in a major bump, they do not cause a major bump inherently. Only Chromium upgrades and major node upgrades cause major bumps, whatever happens to be on master at the time in terms of breaking changes will go out with those major bumps.

I've already held off 3.x upgrades because of churn, and now it's getting worse.

Without making assumptions about your codebase, I always personally recommend making Electron upgrades (or any module upgrade for that matter) in small hops to make it easier instead of waiting and skipping a bunch of versions. We have very strict policies around deprecations warnings and such, skipping versions will skip those warnings.

If you are having issues upgrading your application I'd suggest raising issues detailing any bugs you are experiencing during upgrades (I can't see any issues you've raised so I can't make any assumptions around what's blocking you on the 3.x upgrade). Or going a step further and considering the App Feedback Program

reasonable like a 2 year deprecation period

What you consider reasonable I personally consider way too long, Electron will have gone through ~8+ major releases in that time, probably nearer 14. Leaving a function deprecated in our code base that long is additional maintenance burden that doens't make sense for us to take on board when we have much more important things to be focusing on like pushing the project forward with Chromium upgrades.

Realize that Electron 2.x is less than 6 months old. Six months. In other words, there is a large class of your users who have only just moved to 2.x. Expecting users to see the notices in 3.x is really not a viable option, since many legacy users (the ones who are actually using this API) probably haven't even moved to 3.x yet.

All this said we do hear the timeline concern raised here and we'll discuss it at our next meeting, no guarantees or anything but we'll have a discussion.

lsegal · 2018-10-30T04:28:35Z

If tracking down these kinds of changes are proving tricky I'd suggest considering a type system in your codebase (for example Typescript which we output type definitions for and therefore would catch any and all API changes for you).

I can confirm that TypeScript alone doesn't solve this problem or we wouldn't be here right now. The issue isn't catching the errors, it's the ability to do something with them. Each of the errors that TypeScript bubbles up still need to be handled individually by diffing against the releases page or breaking changes document and fully understanding what needs to be done and why. This would certainly be a lot easier if Electron provided auto-upgrade migration scripts as is common for other projects that introduce breaking API changes (like Angular or rxjs, for example), but until then, all of this work is manual for downstream users who have zero context going into the changes.

We have a strictly documented Versioning Policy that was written with consensus of the entire maintainer community with feedback from lots of the top Electron apps. We didn't just make it up one day and it is a living document being improved on.

There is nothing in the linked versioning policy that refers to the statement you made re deprecations ("we gave two semver/major cycles with a deprecation warning to move over to the new API"). I don't see that contract encoded at all in there. No statement about warning users, intent to provide migration scripts, the 2-version thing. In fact, "deprecation" only appears one time in that document. The versioning policy document may have been engineered just fine, but it is incomplete if it doesn't discuss how you actually handle deprecations. I would highly recommend encoding a proper policy on how you treat old code, why you choose to remove it, and when, so that your users understand what their maintenance cost is.

What you consider reasonable I personally consider way too long, Electron will have gone through ~8+ major releases in that time, probably nearer 14.

Deprecating a feature does not mean you must remove it in the next major rev. In other words, you should consider having a different timeline for the removal of APIs, even though you may be bound by Chromium to major version bump the package. Many languages, libraries, and codebases have deprecated features that are not removed just because a major is bumped. Nothing is stopping you from reving a major version with a new API but not removing an old deprecated one. This brings me to my next point:

Leaving a function deprecated in our code base that long is additional maintenance burden

The real maintenance cost for a one line alias declaration of a function is so near-zero that it may as well be considered zero cost. I would totally understand the argument you are making if there was a deprecated implementation that required complex branching logic, but that is very much not this case.

All this said we do hear the timeline concern raised here and we'll discuss it at our next meeting, no guarantees or anything but we'll have a discussion.

Thank you! As you may tell, I think this is an important topic to discuss. I would love to see Electron stabilize it's own (non-Chromium) APIs and be more friendly to users who want to invest using the platform longterm. As it stands right now, older apps have to pay a really high maintenance cost to keep up with the many changes. As a sidenote, there doesn't seem to be anything in the versioning policy regarding how many major versions back you will support for security / critical patches, so it's unclear if, for example, Electron 1.x users should still expect security patches once 4.x is released, or if they have to upgrade. I see an "at least 2 branches at all times", but there's no specific language on how EOL is communicated, how much warning people will get, etc.

Anyway, I appreciate you hearing me out.

MarshallOfSound · 2018-10-30T04:44:40Z

There is nothing in the linked versioning policy that refers to the statement you made re deprecations

Sorry about that, deprecation policy and breaking changes are documented in our Breaking Changes document

but that is very much not this case.

In this case no, the wrapper that makes the old API work is minimal. This is an edge case, most deprecations are complicated if branches that sometimes end up inside native code which just gets even more complicated.

but there's no specific language on how EOL is communicated, how much warning people will get, etc.

At the moment we're sending security patches to 1.8, 2.0, 3.0 and 4.0. Our release cycle isn't quite stable/consistent enough for that to be set in stone in a doc though, our release velocity is still increasing as we make build system improvements to keep up with Chromium.

MarshallOfSound added 2 commits May 2, 2018 14:25

Fix spec, we don't need process.nextTick hacks any more

cc3062a

MarshallOfSound added the semver/minor backwards-compatible functionality label May 2, 2018

MarshallOfSound requested review from a team May 2, 2018 04:28

Make deprecation TODO for the return value of makeSingleInstance

24667e1

ckerr reviewed May 2, 2018

View reviewed changes

MarshallOfSound added 2 commits May 3, 2018 11:50

Refactor makeSingleInstance to requestSingleInstanceLock and add appr…

0e83284

…opriate deprecation comments

I swear this isn't tricking the linter

067234e

MarshallOfSound force-pushed the single-instance-event branch from 0869f35 to 067234e Compare May 3, 2018 03:19

Make const

85e3d16

ckerr requested changes May 3, 2018

View reviewed changes

Add deprecation warnings for release, and add to planned-breaking-cha…

d9b6a24

…nges

ckerr approved these changes May 3, 2018

View reviewed changes

codebytere approved these changes May 7, 2018

View reviewed changes

MarshallOfSound merged commit 5b5c161 into master May 7, 2018

MarshallOfSound deleted the single-instance-event branch May 7, 2018 15:29

niik added a commit to niik/electron-typescript-definitions that referenced this pull request May 9, 2018

makeSingleInstance isn't a thing any more

f2edf6b

See electron/electron#12782

This was referenced May 9, 2018

fix: Special case ReadableStream type to mean Node.js ReadableStream electron/typescript-definitions#99

Merged

fix: TypeScript definitions test failing due to use of obsolete makeSingleInstance method electron/typescript-definitions#101

Closed

akshitkrnagpal mentioned this pull request Aug 15, 2018

Replaced deprecated method app.makeSingleInstance() jitsi/jitsi-meet-electron#107

Merged

ronjouch mentioned this pull request Dec 20, 2018

Review and adapt to electron 4.0 / 5.0 / 6.0 breaking changes nativefier/nativefier#727

Closed

New makeSingleInstance API #12782

New makeSingleInstance API #12782

Conversation

MarshallOfSound commented May 2, 2018 • edited by jkleinsc

MarshallOfSound commented May 2, 2018

ckerr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarshallOfSound May 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarshallOfSound commented May 2, 2018

Choose a reason for hiding this comment

ckerr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckerr left a comment

Choose a reason for hiding this comment

codebytere left a comment

Choose a reason for hiding this comment

shesek commented Jul 10, 2018

schontz commented Oct 15, 2018

lsegal commented Oct 30, 2018 • edited

schontz commented Oct 30, 2018

MarshallOfSound commented Oct 30, 2018 • edited

lsegal commented Oct 30, 2018

MarshallOfSound commented Oct 30, 2018 • edited

lsegal commented Oct 30, 2018

MarshallOfSound commented Oct 30, 2018

MarshallOfSound commented May 2, 2018 •

edited by jkleinsc

MarshallOfSound May 2, 2018 •

edited

lsegal commented Oct 30, 2018 •

edited

MarshallOfSound commented Oct 30, 2018 •

edited

MarshallOfSound commented Oct 30, 2018 •

edited