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

First shot: control plugins #3503

Closed
wants to merge 1 commit into from
Closed

First shot: control plugins #3503

wants to merge 1 commit into from

Conversation

tmcw
Copy link
Contributor

@tmcw tmcw commented Oct 14, 2016

Okay, here goes: first shot at #1392

See id-upwards for an example plugin.

This establishes the 'hook' which JavaScript plugins can call by invoking a function, but it does not yet propose how you'd include other JavaScript on the page. This could be loading from npm, local sources, an iD-specific source, etc. Open to options.

Tweet-gif of example in action.

Observations:

  • Control styling will need to adapt to a variable number of controls
  • Need to decide whether to control the panel size & placement or let plugins do that.

@bhousel
Copy link
Member

bhousel commented Nov 4, 2016

Sorry for the delay here, this does deserve a response. I think what you've built is really cool and we should encourage people to build more things like this. Discussing (here or in #1392) about what plugins actually need is the first step. Documenting it in API.md is also important.

I think we should be clear on what things in iD we consider the interface, and then be diligent about following semantic versioning starting with the v2.0 release so that people can actually build plugins without breakage.

The list of things is probably this, even though we don't have it written down anywhere:

  • exported functions
  • emitted events
  • url params
  • identified dom elements (#sidebar, #content, #map, #supersurface, #surface, etc)

It looks like your specific plugin needs a few things more on the iD side: A place attach to like a control (we should make an element identified like #controls), and a hook to register the plugin (it's not clear to me what this hook does - could we instead emit an event?)

@tmcw
Copy link
Contributor Author

tmcw commented Nov 4, 2016

Here's how I'm seeing this:

  • iD Plugins will have multiple types. There should not be one-size-fits-all for a plugin that, say, adds a new operation, versus one that adds a UI element, or that adds a new validation step. This would be the first of multiple plugin interfaces and it would be strict in terms of what it allows.
  • As a result, the rest of this list will be talking about 'this kind of plugin' and not necessarily any other kind.
  • This plugin type relies on three connection points:
    • pluginRegisterControl method to register itself
    • context
    • A HTML element that is provided to the plugin for it to fill.

As such, the plugin would not be allowed access to iD, or any other HTML element - it would not be able to select #surface and do stuff with it. Allowing plugins domain over the existing HTML structure of iD seems like a trap: there would be no way to deal with conflicts, and conflicts would quickly occur.

I'm not sure how an event would work in this case, and why it would be preferable to a plugin - which side would listen to the event? Conceptually I don't see how an event could do the job of either a function, as this currently works, or, potentially assigning to a iD member variable, in the style of node's require hooks.

@bhousel
Copy link
Member

bhousel commented Nov 7, 2016

Ok, we are definitely coming at this from different points of view, but hopefully we can meet in the middle and spec out something useful.

Here are my original thoughts, which I'm totally not sold on - just brainstorming..

iD would have a registerPlugin(definition) method that would accept an object with properties like the plugin name, description, version, author, and where its code lives. iD would create a script tag to load the plugin code into, then when onloaded call an init function that would do whatever the plugin needs to do. Yes, this is dangerous, and we should only load "trusted" plugins, whatever we choose that to mean. My assumption was that plugins would contain code like:

function myPlugin(context) {
  var plugin = {
    init: function() {
      d3.select('#surface').call(addPluginThings);
      context.map().on('drawn.myplugin', updatePluginStuff);
    },
    off: function() {
      context.map().on('drawn.myplugin', null);
    }
  };

  return plugin;
};

So clearly my thinking is influenced by how the rest of the code in iD works, and the "d3" way of doing things. But it sounds kind of similar to what you need for your plugin to work: a registration function, a context, and some html.

To your other points:

As such, the plugin would not be allowed access to iD, or any other HTML element - it would not be able to select #surface and do stuff with it. Allowing plugins domain over the existing HTML structure of iD seems like a trap: there would be no way to deal with conflicts, and conflicts would quickly occur.

We can't really prevent this, right?

I'm not sure how an event would work in this case, and why it would be preferable to a plugin - which side would listen to the event?

Plugins would listen for events emitted by iD. Maybe vice versa too, because a plugin should be able to tell iD to redraw in response to something the user did on the plugin side (tricks like context.map().pan([0,0]) will force a redraw, but that's totally hacky).

@tmcw
Copy link
Contributor Author

tmcw commented Nov 7, 2016

We can't really prevent this, right?

We can't prevent it at the code level, but drawing the line there would mean:

  • If plugins do overreach, we don't provide support for them
  • Only approved elements are documented in the plugin API

This is, notably, modeled after the Chrome extension API, Firefox API, modeled after our troubles with the TileMill plugin system, and also with React's no-reaching-up style system: components own a part of the DOM going down, but cannot affect their siblings or parents.

  1. Allowing plugins to hook into the DOM is fundamentally equivalent to versioning the entire DOM structure, which we currently don't do: if you change the classes on a building in the map, it isn't automatically iD 5.0.0. But plugins, if they have access to the entire DOM, will rely on the entire DOM as an API and thus our current versioning system will become irrelevant since plugins that rely on DOM structures will break on minor and patch changes.
  2. With my plan, plugins are isolated, as well as they can be: they should be able to operate independently of each other with little change for conflict. Allowing access to the DOM means that your plugin that, say, adds a feature by inserting it into the building polygon, will have to deal with another plugin that also decides to mess with that same building polygon - and how do the two play? Both 'own' the DOM, should they avoid ever clearing out elements? What if they modify existing elements? As we found with the TileMill API, complete power like this leads to complete and persistent conflict.

Plugins would listen for events emitted by iD.

Sure, I agree with that... plugins can use the context object to hook into iD's events bus. In your previous message, you seemed to be proposing using events instead of the function hook, which led to my confusion.

@tmcw
Copy link
Contributor Author

tmcw commented Mar 7, 2017

Closing; usable as an example but not going to merge this anytime soon.

@tmcw tmcw closed this Mar 7, 2017
@colegleason colegleason mentioned this pull request Jul 7, 2017
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.

2 participants