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 new event to auto-updater #12619

Merged
merged 2 commits into from Apr 16, 2018

Conversation

Projects
None yet
5 participants
@codebytere
Member

codebytere commented Apr 15, 2018

Resolves #8293.

Adds a before-quit-for-update that's emitted when quitAndInstall is called in AutoUpdater.

/cc @MarshallOfSound

@codebytere codebytere requested review from electron/docs as code owners Apr 15, 2018

@zeke

This comment has been minimized.

Member

zeke commented Apr 15, 2018

This is handy.

Is there any guarantee that the event handler will be able to finish before the app quits?

@ckerr

This seems to be a workaround for a before-quit edge case of it being emitted too late when autoupdating, is that correct?

What's the rationale for adding a new wart into the public API, rather than fixing the emission order of before-quit? Is fixing the emission order that tricky?

Marking as "Request changes" because there's not a "Request info" option

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Apr 15, 2018

Changing the emit order may be possible but it would be a breaking change, and not one I'd be super comfortable doing even with semver. Changing order of events would cause very weird issues in some apps that would make it super hard for devs to figure out IMO.

@codebytere

This comment has been minimized.

Member

codebytere commented Apr 15, 2018

@ckerr yeah, i briefly toyed with changing the emit order but given breaking change concerns outlined above, this seemed like the easiest way to address the issue opener's request :)

@ckerr

This comment has been minimized.

Member

ckerr commented Apr 15, 2018

Yeah, that's fair that it would be a breaking change at this point.

@ckerr

ckerr approved these changes Apr 15, 2018

@codebytere

This comment has been minimized.

Member

codebytere commented Apr 15, 2018

@electron/electrocats should we add more people to updater codeowners? right now it's just @zcbenz and @groundwater which is a bit of a bottleneck 🤔

@zeke

I think the docs need more clarification around expected quit behavior.

@zcbenz

zcbenz approved these changes Apr 16, 2018

The change itself looks good to me.

@zeke

zeke approved these changes Apr 16, 2018

@zeke zeke merged commit 40ff17c into master Apr 16, 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

@zeke zeke deleted the add-before-update-event branch Apr 16, 2018

@zeke

This comment has been minimized.

Member

zeke commented Apr 16, 2018

👌

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