Mapbox-gl-js: Document GL JS plugin architecture

[tmcw]

Right now we have a bunch of competing approaches for how mapbox-gl-js plugins express their dependency on GL-JS.

• peerDependencies
• expecting a global to exist or window.mapboxgl
• dependency with a require

Some plugins require-into mapbox-gl for utilities and thus actually rely on gl js internals.

Core questions

• when should dependencies get a reference to gl-js?
• how do we write a system that works equally well for cdn dependencies and browserify and webpack?

cc @jfirebaugh @tristen @mourner

[tristen]

cc @mcwhittemore as he's looking at making mapboxgl.Control it's own module.

[mcwhittemore]

how do we write a system that works equally well for cdn dependencies and browserify and webpack?

Thats a hard one. The first idea that comes to mind is having mapbox-gl pass in a ref to itself via the addControl function. This would let us say that all plugins shouldn't require mapbox-gl. This introduces some problems with confirming that the plugin works with the current version of mapbox-gl.

While I like the idea above, what I've been doing is moving to peerDependencies as this doesn't take a change to mapbox-gl and lowers the bundle size by removing extra refs to mapbox-gl.

[mourner]

I'd say let's do this:

1. Specify Mapbox GL as peerDependencies.
2. Rely on window.mapboxgl to exist, and warn about including after mapboxgl if it's not available. Not a perfect solution but it worked OK for Leaflet plugins and will work for both Browserify systems and CDN.

@mcwhittemore what's the reason for making mapboxgl.Control a module? Also note that not all plugins are supposed to be controls. Some are new sources, some are additional methods, etc.

[mcwhittemore]

@mourner The idea with making mapbox.Control a module is that it could be required in with little bloat. That said, we went with the peerDependencies route over the making mapbox.Control a module when cleaning up mapbox-gl-draw and I think it worked well.

[jfirebaugh]

Some plugins require-into mapbox-gl for utilities and thus actually rely on gl js internals.

They shouldn't do this. require('mapbox-gl') is the public interface.

how do we write a system that works equally well for cdn dependencies and browserify and webpack?

Ideally, only CDN dependencies rely on a mapboxgl global, and browserify/webpack don't. I'm not aware of any turnkey solutions to this. It seems like it requires an approach like:

• The base code of Mapbox GL JS does not set up a global.
• The base code of a plugin exports a factory function that takes a mapboxgl module as input and returns the plugin module as output.
• There's a special "CDN wrapper" for Mapbox GL JS that requires the base code and sets a mapboxgl global.
• There's a special "CDN wrapper" for each plugin that expects the mapboxgl global to be set, requires the plugin base code, calls the factory function, and extends the mapboxgl global with the result.

[mourner]

That looks like too much hassle if the only problem is not wanting to rely on a global inside browserify/webpack.

[mourner]

They shouldn't do this. require('mapbox-gl') is the public interface.

Yes, but then we need to make sure everything a plugin could rely on is exposed (either publicly or through a "protected" method/property).

[jfirebaugh]

the only problem is not wanting to rely on a global inside browserify/webpack.

We get complaints about Mapbox.js relying on globals (https://github.com/mapbox/mapbox.js/issues/726, https://github.com/mapbox/mapbox.js/issues/1060, https://github.com/mapbox/mapbox.js/issues/253, https://github.com/mapbox/mapbox.js/issues/468). We should expect to get them about Mapbox GL JS too if we follow the same path.

[mourner]

@jfirebaugh most of these are specifically about browserify not working, and we can make sure it works while still relying on a global.

[mcwhittemore]

One problem we've run into with mapbox-gl-draw is that its easy for different version of babel to be being used. Because of this we're talking about bundling gl-draw before we ship it. My hold up with this is that is means that mapbox-gl-js will be bundled with it.

[mourner]

My hold up with this is that is means that mapbox-gl-js will be bundled with it.

Why do we need to bundle with mapbox-gl-js? You can bundle excluding it.

[mcwhittemore]

Oh. Didn't know browserify did this. Thanks

[tmcw]

Relying on mapboxgl being a global doesn't work for webpack's async loading pattern:

define([
  'mapbox-gl',
  'mapbox-gl-directions'
], function(mapboxgl,directions) {
  // failure, never gets here, because mapbox-gl-directions is not guaranteed to initialize
  // after mapbox-gl
});

Given the current situation, I think that:

• Third-party controls should never attach themselves to mapboxgl. If we have an official directions plugin, it should be mapboxDirectionsControl, not mapboxgl.Directions.
• We should not rely on mapboxgl until runtime. Relying on the global is not tenable with the variety of build systems right now.
• mapbox.Control should be most useful as an interface, like IControl, not as a class. Plugins should not try to extend from mapbox.Control.
• If a Control depends on the mapboxgl object, it should rely on it as an argument (invert control), rather than as a global. For instance, mapboxDirections(mapbox)().addTo(map).

[mourner]

@tmcw maybe we could recommend using a different webpack pattern instead of placing such restrictions on the plugin architecture? E.g. this:

define(function(require) {
  var mapboxgl = require('mapbox-gl');
  var mapboxglDirections = require('mapbox-gl-directions');
  // all clear
});

[tmcw]

@mourner The above example is an example of webpack code splitting with define syntax; unfortunately it's not just another pattern, it has a much different runtime behavior - specifically that the define style lets people asynchronously introduce new code to their application.

[mourner]

@tmcw yes, but it can still be achieved if we explicitly require the plugin to be loaded after mapboxgl in application code instead of fully rearchitecting plugins, right? For the code splitting, it would look like this:

require.ensure(["mapbox-gl", "mapbox-gl-directions"], function(require) {
    var mapboxgl = require("mapbox-gl");
    var mapboxglDirections = require('mapbox-gl-directions');
    // all clear
});

[lucaswoj]

I have been pushing things in an architectural direction that does not require sources or plugins to reference mapboxgl.

• Custom layer types will be added at runtime with Map#addCustomLayer per https://github.com/mapbox/mapbox-gl-js/issues/281#issuecomment-247182491
• Custom sources will be added at runtime with Map#addCustomSource per https://github.com/mapbox/mapbox-gl-js/projects/2 (not explicitly documented, this method is icing on the bigger refactor)
• Custom controls will be added at runtime with Map#addControl per https://github.com/mapbox/mapbox-gl-js/issues/2118

If all three of these changes go through, we should be able to sidestep all the architecture problems discussed in this ticket.

Does this direction make sense? Are there any other classes of plugins that should be considered? Is there anything I'm missing?

[tmcw]

So the approach would be to remove the mapboxgl dependency from all plugins? I _think_ that's doable, as long as all Map methods support plain-old javascript objects on all methods - is that the case?

[lucaswoj]

as long as all Map methods support plain-old javascript objects on all methods - is that the case?

I believe this is the case. The only edge case I can think of is a plugin that wants to create instances of controls, Popups, and Markers before receiving a reference to the map.

[tmcw]

As of https://github.com/mapbox/mapbox-gl-js/pull/3497 the code and API documentation is in place! But I'd like to queue up a blog post explaining this new API once a new release is rolled.

Also tracking updates for our controls:

• [x] https://github.com/mapbox/mapbox-gl-draw/issues/539
• [x] https://github.com/mapbox/mapbox-gl-directions/issues/94
• [x] https://github.com/mapbox/mapbox-gl-geocoder/issues/67

[tmcw]

@lucaswoj geocoder is now finished. Which branch should plugins/ directory PRs target?

[mollymerp]

@tmcw it should target mb-pages so the examples are fixed asap

[tmcw]

I'm going to write up a blog about the plugin architecture and then we can close this issue.

[tmcw]

Blogged, done https://www.mapbox.com/blog/build-mapbox-gl-js-plugins/