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

Package URL handlers - mapping atom:// URIs to package functions #11399

Merged
merged 43 commits into from Oct 17, 2017

Conversation

Projects
None yet
@BinaryMuse
Member

BinaryMuse commented Apr 7, 2016

See update below: #11399 (comment)


This PR implements the MessageRegistry, which is similar to the CommandRegistry in that it maps string types of some kind to callback functions, but it differs in that messages originate from outside the application via atom:// URIs handled via the open-url event.

This partially implements #5262, provides a mechanism for hitting things like atom/settings-view#614 from outside the application (e.g. "View in Atom" links on package pages on the Atom site), and would enable package authors to implement things like #2037 for external debuggers, etc.

There is still some additional work to do around registering Atom as a system-level handler for atom:// URIs on non-Mac systems, but that probably will be installer-based work.

/cc @atom/core @atom/feedback for review. Some open questions I have are:

  • I restricted messages to URIs starting with atom://atom as we already have a mechanism that utilizes URIs with hosts that match package names to perform some work. (What is this used for? I'm not familiar with it.) I'd love to get rid of this, as it means we can't use any host that might match a package name. If anyone has bright ideas for URI schemes, that'd be awesome. (I thought maybe something like atom://.message/...?)
  • Right now, the callback function is called with the string message (just the namespace:name portion) and the params, but maybe it makes more sense to pass the entire URI, or the parsed URI object.
@lee-dohm

This comment has been minimized.

Show comment
Hide comment
@lee-dohm

lee-dohm Apr 8, 2016

Member

already have a mechanism that utilizes URIs with hosts that match package names to perform some work. (What is this used for? I'm not familiar with it.)

This is used for importing resources into Less without needing to know the path where packages are stored: https://github.com/lee-dohm/red-wavy-underline/blob/a7c36b5b8790016f81f7cf48d840ed44333df7fa/styles/red-wavy-underline.less#L2

Member

lee-dohm commented Apr 8, 2016

already have a mechanism that utilizes URIs with hosts that match package names to perform some work. (What is this used for? I'm not familiar with it.)

This is used for importing resources into Less without needing to know the path where packages are stored: https://github.com/lee-dohm/red-wavy-underline/blob/a7c36b5b8790016f81f7cf48d840ed44333df7fa/styles/red-wavy-underline.less#L2

@zeke

This comment has been minimized.

Show comment
Hide comment
@zeke

zeke Apr 8, 2016

Member

red-wavy-underline is a great package name.

Member

zeke commented Apr 8, 2016

red-wavy-underline is a great package name.

@BinaryMuse

This comment has been minimized.

Show comment
Hide comment
@BinaryMuse

BinaryMuse Apr 11, 2016

Member

It looks like electron/electron#4896 (comment) took care of the work for allowing apps to be protcol handlers on OS X and Windows. Linux is still an open question for me.

Member

BinaryMuse commented Apr 11, 2016

It looks like electron/electron#4896 (comment) took care of the work for allowing apps to be protcol handlers on OS X and Windows. Linux is still an open question for me.

@BinaryMuse

This comment has been minimized.

Show comment
Hide comment
@BinaryMuse

BinaryMuse Apr 11, 2016

Member

This is used for importing resources into Less without needing to know the path where packages are stored

@lee-dohm and I discussed this and it's a slightly different thing.

Member

BinaryMuse commented Apr 11, 2016

This is used for importing resources into Less without needing to know the path where packages are stored

@lee-dohm and I discussed this and it's a slightly different thing.

@thedaniel

This comment has been minimized.

Show comment
Hide comment
@thedaniel

thedaniel Apr 18, 2016

Contributor

It looks like electron/electron#4896 (comment) took care of the work for allowing apps to be protcol handlers on OS X and Windows.

Does that mean this PR is now waiting on an Electron upgrade to 0.37.4+?

Contributor

thedaniel commented Apr 18, 2016

It looks like electron/electron#4896 (comment) took care of the work for allowing apps to be protcol handlers on OS X and Windows.

Does that mean this PR is now waiting on an Electron upgrade to 0.37.4+?

@BinaryMuse

This comment has been minimized.

Show comment
Hide comment
@BinaryMuse

BinaryMuse Apr 30, 2016

Member

@iolsen This is just a reminder ping that we need to chat about this. :)

Member

BinaryMuse commented Apr 30, 2016

@iolsen This is just a reminder ping that we need to chat about this. :)

@BinaryMuse

This comment has been minimized.

Show comment
Hide comment
@BinaryMuse

BinaryMuse May 1, 2016

Member

@thedaniel That's correct; we would get this from #11474, which we can move forward on now that #11348 has been merged.

It looks like setting a default protocol handler via the Electron API is not supported by Linux; I'm actually not sure what it would take to do such a thing on Linux, or if it's even reasonable to be able to do so in a way that works for everyone.

Member

BinaryMuse commented May 1, 2016

@thedaniel That's correct; we would get this from #11474, which we can move forward on now that #11348 has been merged.

It looks like setting a default protocol handler via the Electron API is not supported by Linux; I'm actually not sure what it would take to do such a thing on Linux, or if it's even reasonable to be able to do so in a way that works for everyone.

BinaryMuse added some commits Apr 7, 2016

Add MessageRegistry
The MessageRegistry is similar to the CommandRegistry in that it maps
string message types to callback functions; however, messages are
dispatched to Atom from outside the application using URIs, and so
should be more restrictive about what they allow.

Signed-off-by: Katrina Uychaco <kuychaco@github.com>
@bastilian

This comment has been minimized.

Show comment
Hide comment
@bastilian

bastilian Jun 8, 2016

@BinaryMuse I did a bit of digging around and it seems that xdg-utils is the best way to add a protocol handler on Linux, which seems to be used or integrated in most popular desktop environments XFCE, KDE and Gnome.

bastilian commented Jun 8, 2016

@BinaryMuse I did a bit of digging around and it seems that xdg-utils is the best way to add a protocol handler on Linux, which seems to be used or integrated in most popular desktop environments XFCE, KDE and Gnome.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Jul 26, 2016

Contributor

I'd prefer a name like "external URL handler" or something similar. My concern about the term "message registry" is that it's a bit general, and presumably there are other ways to message the application such as an OS-provided RPC facility. A name that more closely mirrors the actual implementation might be clearer.

I would definitely like to figure out if we're using the existing top-level namespace for anything critical. Even if we are, if we could move those URLs to something like atom://packages/my-package, I'd prefer it. Just like for services, I think we should encourage users to namespace their external URL handlers based on their package name... so for example if I had a package named foo, I would register the namespace atom://foo/.... I also think the handler should take the entire URL. Perhaps absent the atom:// prefix?

As for registering these handlers, I think we should consider a setup where handlers are registered statically via the package.json. We've moved in this direction with other global facilities in Atom. The big advantage to doing something static is that you don't have to activate the package or evaluate any code to know a handler has been registered. This makes it easier to defer loading for packages that only define these kinds of handlers.

Obviously, some amount of arbitrary code execution might be required to interpret a given URL, but I don't think it would be too onerous to require packages to at least register for a specific URL prefix.

Contributor

nathansobo commented Jul 26, 2016

I'd prefer a name like "external URL handler" or something similar. My concern about the term "message registry" is that it's a bit general, and presumably there are other ways to message the application such as an OS-provided RPC facility. A name that more closely mirrors the actual implementation might be clearer.

I would definitely like to figure out if we're using the existing top-level namespace for anything critical. Even if we are, if we could move those URLs to something like atom://packages/my-package, I'd prefer it. Just like for services, I think we should encourage users to namespace their external URL handlers based on their package name... so for example if I had a package named foo, I would register the namespace atom://foo/.... I also think the handler should take the entire URL. Perhaps absent the atom:// prefix?

As for registering these handlers, I think we should consider a setup where handlers are registered statically via the package.json. We've moved in this direction with other global facilities in Atom. The big advantage to doing something static is that you don't have to activate the package or evaluate any code to know a handler has been registered. This makes it easier to defer loading for packages that only define these kinds of handlers.

Obviously, some amount of arbitrary code execution might be required to interpret a given URL, but I don't think it would be too onerous to require packages to at least register for a specific URL prefix.

@jgebhardt

This comment has been minimized.

Show comment
Hide comment
@jgebhardt

jgebhardt Aug 25, 2016

Contributor

Just wanted to re-iterate that having this functionality would be huge. I already used it to prototype a few use cases for potential handlers in Nuclide, and it would enable some very powerful integrations.

@nathansobo's point about renaming the API makes sense, since MessageRegistry is not super intuitive imho.

Other than deciding on the name, registry, and namespacing convention, is anything else (maybe the Linux issue?) blocking this from making it into a release?

Contributor

jgebhardt commented Aug 25, 2016

Just wanted to re-iterate that having this functionality would be huge. I already used it to prototype a few use cases for potential handlers in Nuclide, and it would enable some very powerful integrations.

@nathansobo's point about renaming the API makes sense, since MessageRegistry is not super intuitive imho.

Other than deciding on the name, registry, and namespacing convention, is anything else (maybe the Linux issue?) blocking this from making it into a release?

@DavidSnider

This comment has been minimized.

Show comment
Hide comment
@DavidSnider

DavidSnider Aug 28, 2016

If we could make this happen, I would be suuuper happy

DavidSnider commented Aug 28, 2016

If we could make this happen, I would be suuuper happy

@bolinfest

This comment has been minimized.

Show comment
Hide comment
@bolinfest

bolinfest Oct 9, 2016

Contributor

@BinaryMuse Any chance you'd be willing to pick this up again soon?

Contributor

bolinfest commented Oct 9, 2016

@BinaryMuse Any chance you'd be willing to pick this up again soon?

@bolinfest

This comment has been minimized.

Show comment
Hide comment
@bolinfest

bolinfest Oct 9, 2016

Contributor

I agree with @nathansobo's point that these should be able to be registered statically. I think that this could also sidestep the conversation about the MessageRegistry naming for now. We would only have to bikeshed the name of the property in the package.json :).

In that vein, it could conceivably be exposed as a service, though up until now, I don't believe that services could include their own static configuration (though really, why not)?

Contributor

bolinfest commented Oct 9, 2016

I agree with @nathansobo's point that these should be able to be registered statically. I think that this could also sidestep the conversation about the MessageRegistry naming for now. We would only have to bikeshed the name of the property in the package.json :).

In that vein, it could conceivably be exposed as a service, though up until now, I don't believe that services could include their own static configuration (though really, why not)?

@bolinfest

This comment has been minimized.

Show comment
Hide comment
@bolinfest

bolinfest Oct 9, 2016

Contributor

Actually, how bad would it be if it were just a service and you couldn't specify the prefix? That is, what if you were forced to use a prefix that matched the name of your extension? So if you had the following as your extension's package.json:

{
  "name": "an-example",
  "providedServices": {
    "atom.url-handler": {
      "versions": {
        "1.0.0": "onAtomURI"
      }
    }
  }
}

Then if Atom received a URI of the form atom://url-handler/an-example/..., then an-example would be activated and the URI would be passed as an argument to the registered provider, onAtomURI().

Contributor

bolinfest commented Oct 9, 2016

Actually, how bad would it be if it were just a service and you couldn't specify the prefix? That is, what if you were forced to use a prefix that matched the name of your extension? So if you had the following as your extension's package.json:

{
  "name": "an-example",
  "providedServices": {
    "atom.url-handler": {
      "versions": {
        "1.0.0": "onAtomURI"
      }
    }
  }
}

Then if Atom received a URI of the form atom://url-handler/an-example/..., then an-example would be activated and the URI would be passed as an argument to the registered provider, onAtomURI().

@hansonw

This comment has been minimized.

Show comment
Hide comment
@hansonw

hansonw Oct 21, 2016

Contributor

+1 to @mbolin's suggestion. Maybe we could even simplify things a little and have atom://package-name/path?params trigger package-name's URL handler service callback?

From searching on github, there's only three package.jsons that contain a urlMain - one of them looks experimental and another is a homedir backup, so there's only one package (https://atom.io/packages/mastermind) that actually relies on this behaviour. (We could preserve the old urlMain behaviour if no path is specified).

No matter what we go with, this has tremendous value to internal Facebook users so I'd be willing to try and push this through myself if @BinaryMuse is busy :P

Contributor

hansonw commented Oct 21, 2016

+1 to @mbolin's suggestion. Maybe we could even simplify things a little and have atom://package-name/path?params trigger package-name's URL handler service callback?

From searching on github, there's only three package.jsons that contain a urlMain - one of them looks experimental and another is a homedir backup, so there's only one package (https://atom.io/packages/mastermind) that actually relies on this behaviour. (We could preserve the old urlMain behaviour if no path is specified).

No matter what we go with, this has tremendous value to internal Facebook users so I'd be willing to try and push this through myself if @BinaryMuse is busy :P

facebook-github-bot pushed a commit to facebook/nuclide that referenced this pull request Nov 7, 2016

Create Nuclide urlMain hook to handle atom://nuclide URLs
Summary:
This enables atom://nuclide/path?param=1 URLs to be intercepted and processed from within Nuclide.

atom/atom#11399 is an open PR on the Atom side, but likely requires some work and it'll be a while before this is ready. I plan to work on a new proposal to drive that forward, but in the meantime this leverages the existing `urlMain` functionality that Atom provides to make it work.

A quick rundown of how this works:

- Atom provides a `package.json`-level `urlMain` hook. If provided, `atom://{package}` URIs launch your `urlMain` code rather than the standard window initialization script: https://github.com/atom/atom/blob/master/src/main-process/atom-application.coffee#L627
- This adds a `urlMain` hook for Nuclide. There are two cases to handle:

This comes with tests to ensure that the core functionality continues to work across Atom versions.
It's by no means complete, but I imagine it should hold us over until we can upstream a saner method.

Reviewed By: zertosh

Differential Revision: D4085694

fbshipit-source-id: b4f091cf53d2e0d04d5cb1f6d13fbc38da2d3846

BinaryMuse added some commits Sep 18, 2017

@nathansobo

I'm not the world's best asynchronous code reviewer, but this all looks reasonable to me. On the pedantic naming front, I'd like to consistently use the term "URI" over URL since there's precedent for using the name "URI" elsewhere in the API, and also we should always capitalize the full initialism in camel-cased words. The only major question I have is about the ordering of package activation and URL handlers. Seems like the package should always activate first. Maybe that's what it does already and I'm misunderstanding, or maybe there's a good reason not to do that I'm not thinking of.

Also, where are we planning on documenting the new package.json features? I want to be sure documentation gets updated in sync with this feature.

Thanks for adding this! It's going to be useful in a variety of places.

Show outdated Hide outdated spec/url-handler-registry-spec.js
Show outdated Hide outdated spec/url-handler-registry-spec.js
Show outdated Hide outdated src/atom-environment.coffee
Show outdated Hide outdated src/atom-environment.coffee
@@ -55,6 +55,25 @@ const configSchema = {
}
}
},
uriHandlerRegistration: {

This comment has been minimized.

@nathansobo

nathansobo Oct 17, 2017

Contributor

More naming feedback... I'm seeing a mix of "URL" and "URI". I prefer URI myself because I think it's a more general concept.

@nathansobo

nathansobo Oct 17, 2017

Contributor

More naming feedback... I'm seeing a mix of "URL" and "URI". I prefer URI myself because I think it's a more general concept.

Show outdated Hide outdated src/main-process/atom-application.coffee
Show outdated Hide outdated src/main-process/atom-application.coffee
Show outdated Hide outdated src/package.coffee

BinaryMuse added some commits Oct 17, 2017

@BinaryMuse

This comment has been minimized.

Show comment
Hide comment
@BinaryMuse

BinaryMuse Oct 17, 2017

Member

@nathansobo I switched up the naming across the three relevant PRs (two of them in settings-view).

Documentation will be added to the flight manual as part of this effort (see the URI Handling project for more).

Member

BinaryMuse commented Oct 17, 2017

@nathansobo I switched up the naming across the three relevant PRs (two of them in settings-view).

Documentation will be added to the flight manual as part of this effort (see the URI Handling project for more).

Addressed requested changes

@nathansobo

🚢 when ready!

@iolsen

This comment has been minimized.

Show comment
Hide comment
@iolsen

iolsen Oct 17, 2017

Contributor

Some damn fine work pushing this monster effort over the finish line, @BinaryMuse! 💯

Contributor

iolsen commented Oct 17, 2017

Some damn fine work pushing this monster effort over the finish line, @BinaryMuse! 💯

@BinaryMuse BinaryMuse referenced this pull request Oct 17, 2017

Merged

Add docs for URI handling #385

2 of 2 tasks complete

@BinaryMuse BinaryMuse merged commit d6a3a60 into master Oct 17, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@BinaryMuse BinaryMuse deleted the mkt-url-based-command-dispatch branch Oct 17, 2017

@BinaryMuse BinaryMuse referenced this pull request Oct 19, 2017

Closed

Allows atom to open local files by URL scheme #14243

2 of 2 tasks complete

@BinaryMuse BinaryMuse referenced this pull request Nov 15, 2017

Closed

Make the portal ID a link that opens in Atom #109

8 of 8 tasks complete
@akonwi

This comment has been minimized.

Show comment
Hide comment
@akonwi

akonwi Dec 13, 2017

Contributor

This is great and I'm excited to use this. My biggest question right now is whether this means an atom://cool-package link will go to the package details in atom settings to allow the user to install it if it isn't already installed. From what I gathered skimming this thread, this api is more for already installed packages responding to URIs

Contributor

akonwi commented Dec 13, 2017

This is great and I'm excited to use this. My biggest question right now is whether this means an atom://cool-package link will go to the package details in atom settings to allow the user to install it if it isn't already installed. From what I gathered skimming this thread, this api is more for already installed packages responding to URIs

@BinaryMuse

This comment has been minimized.

Show comment
Hide comment
@BinaryMuse

BinaryMuse Dec 13, 2017

Member

@akonwi settings-view sets up a URI handler that can show the installation/details page for a package; see atom/settings-view#1011

Member

BinaryMuse commented Dec 13, 2017

@akonwi settings-view sets up a URI handler that can show the installation/details page for a package; see atom/settings-view#1011

@akonwi

This comment has been minimized.

Show comment
Hide comment
@akonwi

akonwi Dec 14, 2017

Contributor

thanks!

Contributor

akonwi commented Dec 14, 2017

thanks!

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