Custom plugins + bug fixes

[gabrielalmeida]

Hello,

This PR probably should be divided into multiples, sorry for this.

• Fixes a loading issue that occurs while using multiples plugins
• Adds API options to load custom plugins
• Adds few test cases
• Updates the README.md to reflect new API
• Bumps version

Check the diff and commits for more explanations.

I think just having method leafletMaps.addPlugins() at docs is enough to allow users to handle plugins. It's probably better omit the first method(writing directly to Meteor.settings).

Let me know if you think there are things that need to be changed before merging.

Thanks for this package!

[mattkrick]

I like the approach! Here a couple quick thoughts:

We should keep isLoaded ReactiveVar for backwards compatibility, otherwise it’ll be a breaking change.

Instead of using Meteor.settings, we could use the leafletMaps global var to keep everything self contained. Thoughts?

I like the defaults, but if those files don’t exist, we get 2 console messages (for CSS and JS): Resource interpreted as Stylesheet but transferred with MIME type text/html Uncaught SyntaxError: Unexpected token < I know JS is limited in file reads, but can you think of any tricks we could perform to make sure the defaults are valid? If not, we may just have to go without defaults.

The __pluginsReady is definitely the right way to do things (I wouldn’t trust the old way if I had to load a ton of plugins) but intervals scare me. Could we replace the interval with something like:

Tracker.autorun(function(comp) {
  if (leafletMaps.__pluginsReady.get() === jsFiles.length) {
    leafletMaps.isLoaded.set(true);
    comp.stop();
  }
})

Let me know whatcha think.

[gabrielalmeida]

I agree on almost everything you've said.

We should keep isLoaded ReactiveVar for backwards compatibility, otherwise it’ll be a breaking change

Surely isLoaded should be maintained as deprecated

Instead of using Meteor.settings, we could use the leafletMaps global var to keep everything self-contained. Thoughts?

I also agree on that, it's probably a better approach to keep plugins stored inside leafletMaps itself

I like the defaults, but if those files don’t exist, we get 2 console messages (for CSS and JS): ...

Yep, I was aware of those. Not so much to do while using plain script.src to load the file I believe, by using xhr any defaults would always return a 200 on xhr.status, so the only way to inspect the file reading is by catching the Content-Type header returned and check if it was served as "application/javascript", "text/css" or in the case of an unknown route(file not found) as "text/html"(This is what meteor will return if the URL doesn't match a static file on public folder). So it's kinda possible to do, although a little bit tricky..

But I think defaults as it is now should not exist. At the end, all it's is doing is defining some filenames for the user. Unless you are aiming to do something like download the files and handle it automagically to the user like doing now with leaflet.js from CDN, I see it as something unnecessary that only brings complexity.

The __pluginsReady is definitely the right way to do things (I wouldn’t trust the old way if I had to load a ton of plugins) but intervals scare me. Could we replace the interval with something like:

I can't tell about any performance differences here and can't see caveats on using interval, maybe Tracker could be a bit overkill in this case(?). But, as it's the Meteor standard and probably "fails better" than scary setInterval, It's probably an option to consider. I guess with some research it's possible to correctly choose the "best" one.

I will update the PR as we advance on what to do in each case.

[mattkrick]

Nice! So let's use the leafletMaps namespace for the pluginDict object. As for JSON vs addPlugins, I'd say the addPlugins method is preferred & the user could stick it in the Meteor.onStartup callback. (https://forums.meteor.com/t/how-do-you-guys-handle-config-across-app-and-many-packages/7244). Here's my idea, addPlugins creates the entire leafletMaps.plugins object, but doesn't load anything.

leafletMaps.plugins = {
  awesomeMarkers: {
    cssFiles: [...],
    jsFiles: [...],
    isLoaded: false
  }
}

Then, the map is accessed, it checks the plugins that that specific map requires, and if the pluginLoaded flag is false, it loads them and sets the flag to true. That way, if the user has 1 particular map that is rarely accessed but requires a ton of plugins, the other maps in his app won't have to load all the plugins if they're accessed first. What do you think?

For setInterval vs. Tracker I'm OK with using setInterval but there's no need to use a ReactiveVar (and we should set a failsafe break after 10 seconds in case something weird happens).

Let me know if there's anything you want me to code, I'm pretty free this week.