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

Add "app.whenReady()" #12652

Merged
merged 2 commits into from Apr 20, 2018

Conversation

Projects
None yet
5 participants
@alexeykuzmin
Contributor

alexeykuzmin commented Apr 18, 2018

app.ready getter that would return a Promise didn't look very well aligned with the rest on the Electron API, so I've decided to go with the app.whenReady() method instead.

Closes #9561.
/cc @sindresorhus

@alexeykuzmin alexeykuzmin requested review from electron/docs as code owners Apr 18, 2018

@YurySolovyov

This comment has been minimized.

Contributor

YurySolovyov commented Apr 18, 2018

Is there a case when ready never fires due to error or something?

@@ -432,6 +432,10 @@ app.exit(0)
Returns `Boolean` - `true` if Electron has finished initializing, `false` otherwise.
### `app.whenReady()`
Returns `Promise` - fulfilled when Electron is initialized.

This comment has been minimized.

@sindresorhus

sindresorhus Apr 18, 2018

Contributor

Maybe also mention that this is an alternative to app.isReady() and app.once('ready'). Could be confusing to people to know the difference.

@ckerr

ckerr approved these changes Apr 18, 2018

Agree with @sindresorhus about fleshing out the docs, but otherwise LGTM

Show resolved Hide resolved spec/api-app-spec.js
@MarshallOfSound

In general, I'd rather we use the native promise implementation coming in electron/native-mate#32

I'd like to discuss this before merging if possible as this PR is the first real promise based thing going into Electron and I'd like to set the guidelines / standards on what we want to do here 😄

readyPromise = Promise.resolve()
} else {
readyPromise = new Promise(resolve => {
app.once('ready', resolve)

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Apr 18, 2018

Member

This will leak a tiny amount of memory, should switch to app.once('ready', () => resolve()) to ensure the launchInfo isn't left hanging around in this promise that will never be GC'ed.

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Apr 18, 2018

Contributor

Done.

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Apr 18, 2018

Updated the docs.

### `app.whenReady()`

Returns `Promise` - fulfilled when Electron is initialized.
May be used as a convenient alternative to checking `app.isReady()`
and subscribing to the `ready` event if the app is not ready yet.
@MarshallOfSound

Requesting discussion 😄

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Apr 18, 2018

In general, I'd rather we use the native promise implementation coming in electron/native-mate#32

I'd like to discuss this before merging if possible as this PR is the first real promise based thing going into Electron and I'd like to set the guidelines / standards on what we want to do here 😄

There is no callback based API for this, so I don't think it is really related to The Promisification.

@YurySolovyov

This comment has been minimized.

Contributor

YurySolovyov commented Apr 18, 2018

I'd like to set the guidelines / standards on what we want to do here

That would be awesome. Also, maybe a little guide on how to implement/expose Promise-based API.

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Apr 18, 2018

Is there a case when ready never fires due to error or something?

@YurySolovyov It can happen. By it would mean something terrible has happened, and the app is not usable at all. So there are not reasons to handle this case.

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Apr 18, 2018

That would be awesome. Also, maybe a little guide on how to implement/expose Promise-based API.

That would be a good thing to have, I agree. But it seems to be not ready yet.

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Apr 18, 2018

@MarshallOfSound Anything else to change or any topics to discuss?

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Apr 20, 2018

Anything else to change or any topics to discuss?

Will let this one through as you're right, it isn't directly related to the promisification work 👍

That said, once we have native promises I'd probably look to reimplement this one first in native promises (would make a good test bed for sure)

@MarshallOfSound MarshallOfSound merged commit fcc82eb into master Apr 20, 2018

10 checks passed

WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-mas-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-osx-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@MarshallOfSound MarshallOfSound deleted the add-app-when-ready branch Apr 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment