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: convert shell.openItem to async shell.openPath #20682

Merged
merged 1 commit into from
Nov 8, 2019

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Oct 22, 2019

Description of Change

BREAKING CHANGE.

Resolves #20606.
Resolves #7210.

Implements an asynchronous version of shell.openItem(path) as a new method shell.openPath(path).

Adheres to this spec.

Checklist

Release Notes

Notes: Split shell.openItem(path) into synchronous and asynchronous methods.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Oct 22, 2019
@deepak1556
Copy link
Member

deepak1556 commented Oct 22, 2019

@codebytere this will conflict #10902 in behavior. /cc @zcbenz @ckerr

@codebytere
Copy link
Member Author

@deepak1556 hmm - why don't we allow in an optional callback with an error code and description string if one exists? i think that would solve both issues 🤔

@deepak1556
Copy link
Member

There is no reason to not implement it #7210 😄

@codebytere codebytere force-pushed the open-item-should-be-accurate branch 2 times, most recently from ef0db35 to e2be35a Compare October 23, 2019 02:07
@codebytere codebytere changed the title fix: shell.openItem should wait for exit code [wip] feat: add optional callback to shell.openIte m Oct 23, 2019
@codebytere codebytere changed the title [wip] feat: add optional callback to shell.openIte m [wip] feat: add optional callback to shell.openItem Oct 23, 2019
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Oct 23, 2019
@codebytere codebytere changed the title [wip] feat: add optional callback to shell.openItem [wip] feat: make shell.openItem async Oct 23, 2019
@codebytere codebytere changed the title [wip] feat: make shell.openItem async [wip] feat: split shell.openItem into sync and async Oct 23, 2019
@codebytere codebytere force-pushed the open-item-should-be-accurate branch 6 times, most recently from e335b9f to 23dfb54 Compare October 23, 2019 20:56
@codebytere codebytere changed the title [wip] feat: split shell.openItem into sync and async feat: split shell.openItem into sync and async Oct 23, 2019
shell/common/api/atom_api_shell.cc Outdated Show resolved Hide resolved
shell/common/platform_util_linux.cc Outdated Show resolved Hide resolved
shell/common/platform_util_mac.mm Outdated Show resolved Hide resolved
shell/common/platform_util_mac.mm Outdated Show resolved Hide resolved
docs/api/shell.md Outdated Show resolved Hide resolved
@zcbenz
Copy link
Member

zcbenz commented Oct 24, 2019

Generally I think we should not change API's behavior and return value silently, otherwise we would not be able to show deprecation warnings, and it would very hard to users to debug if any bug was introduced to their apps due to the behavior change.

For example, previous silent errors would now turn into unhandled exceptions.

A safe choice would be deprecating openItem and renaming the APIs to openPath/openPathSync.

/cc @electron/wg-api for more inputs.

@codebytere
Copy link
Member Author

codebytere commented Oct 24, 2019

@zcbenz fair point! i'll address these mechanical changes and then add this to the agenda for tomorrow :)

Do you think we should get rid of the sync version entirely? I personally would prefer that as I think the sync version leads to false assumtions for users about the nature of the synchronicity.

@codebytere codebytere force-pushed the open-item-should-be-accurate branch 5 times, most recently from 870166d to d83a000 Compare November 2, 2019 01:30
@codebytere
Copy link
Member Author

This should be ready to go per the spec!

shell/common/platform_util_linux.cc Outdated Show resolved Hide resolved
@codebytere codebytere merged commit d3622f9 into master Nov 8, 2019
@release-clerk
Copy link

release-clerk bot commented Nov 8, 2019

Release Notes Persisted

Split shell.openItem(path) into synchronous and asynchronous methods.

@codebytere codebytere deleted the open-item-should-be-accurate branch November 8, 2019 07:09
@vladimiry vladimiry mentioned this pull request Dec 27, 2019
3 tasks
LabhanshAgrawal added a commit to LabhanshAgrawal/hyper that referenced this pull request May 29, 2020
Stanzilla pushed a commit to vercel/hyper that referenced this pull request May 29, 2020
@anitakurz
Copy link

The release-notes of this breaking change says: Split shell.openItem(path) into synchronous and asynchronous methods.
This is misleading as openItem was replaced by openPath. The link to the spec in the first comment is also broken (not https://github.com/electron/governance/blob/master/wg-api/RFCs/shell-openitem.md anymore but https://github.com/electron/governance/blob/master/wg-api/spec-documents/shell-openitem.md). This is important as the API-documentation in this change is wrong and got corrected with #21641 (which is not linked anywhere).
Comming from https://www.electronjs.org/releases/stable#breaking-changes it is quite hard to find out what has been going on. I'd be glad if the release notes of a breaking change would state the nature of the change more clearly and/or outline the actions required for a successful upgrade.
Thanks for considering it.

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

Successfully merging this pull request may close these issues.

shell.openItem returns always true on Linux shell.openItem() is not async
4 participants