Support setup to return Promise

[Aradiv]

This Pull request is related to #76

Currently a setup function that returns a promise will be executed "sometimes" after the loadPlugins() call but the load plugins call doens't wait till the promise is resolved.

This change allows a setup function to return a Promise and have the setup function be executed at the same time as other setup functions that have the same priority.

It also maintains the execution order of the other parts so for example layer selection code doens't run before the map layers are added by the plugin.

This change is also compatible with normal setup functions so plugins do not need to change to promises but can use whatever style they like.

Possible Improvements:

• load plugins in parallel currently all normal setup functions run sequenqually in a single wrapper Promise. We could change this by wrapping each plugin in his own wrapper

• evaluate the results of Promise.allSettled() currently it we do not react at all to a plugin failing to load.

[johnd0e]

1. You should avoid mixing main changes with whitespace changes
2. https://github.com/IITC-CE/ingress-intel-total-conversion/wiki/Code-Style

[johnd0e]

Have I understood correctly that the only reason of proposed changes is performance gain from parallel plugins' setup execution?

[Aradiv]

No Plugins that use Promises are currently executed and some undetermined time after the setup call.

This can have unwanted side effects.

One example if you add a BaseLayer, that was previously selected IITC, will not select the Layer again since it isn't available when the selection runs because the call hasn't finished yet so you have to run the selection code again in the setup.

With this change the execution order of things is mainated also for plugins that want to use Promises

[johnd0e]

Plugins that use Promises [...]

You mean that some plugins use promises internally for own purposes.

One example if you add a BaseLayer

We had 2 similar plugins: Bing and Yandex.
Till last release their layers were additionally wrapped into L.LayerGroup.

So 2 questions:

1. Could you name some real plugins that would be gained from this change?
2. Is it limited to baselayers or may be you have more use cases on your mind?

[Aradiv]

Every Plugin that want to use Promises (currently they can't) and relies on any of the lines after the loadPlugin(); call in bootjs to actually run after the setup

This gets worse if you have a "boot" plugin because their is just more stuff to run later.

[johnd0e]

'Any' is good. And I support your idea.
But in order to estimate it's priority I'd like to see some real examples.

And I'd prefer not be too quick on implementation, as we used to have differing plans for values returned from setup.

[Aradiv]

i created a quick plugin that makes the issue visible.
https://gist.github.com/Aradiv/978e3071c5203f6dedd25d7543d9aaa5

it is a copy of the openstreetmap plugin but with an added delay till the layers are available to simulate more work beeing done (otherwise the setup might be to fast to recognize))

[McBen]

Currently a setup function that returns a promise will be executed "sometimes" after the loadPlugins() call but the load plugins call doens't wait till the promise is resolved.

A plugin that requires a delay in boot time is a bad plugin. So this behaviour is just fine.

Promises are for event handling not for multithreading.

[johnd0e]

I am not so categorical as @McBen, but still I am not convinced enough that we absolutely need some special support for promises.
Our base lib - Leaflet - is not promises-driven, so every it's plugin written with that consideration.

i created a quick plugin that makes the issue visible.

Look our current Google, Bing and Yandex - all of them have deferred initialization.
Do you have any issues with them?

P.S.
But I do not reject the idea.

[Aradiv]

The actual execution time of the real script is really fast the only Problem is that it expect things like the iitcLoaded hook or the basemap layer selection to be run when the loading of everything is done and not before.

And the Problem is that i have to load data async before the setup is finished. (indexedDB)

The Problem with wrong loading occures with the real plugin arround 50% of the time.

The Problem is that you basically loose all ability to guaranteed run before setup is finished when you do anything that is async like load from indexdb/AJAX requests etc etc.

The long delay in the demo plugin is only to showcase the problem and guarantee it will happen every time.

[johnd0e]

that i have to load data async before the setup is finished. (indexedDB)

Are you sure that you have to do it before setup returned?
Please provide real example to avoid pointless discussion.

[Aradiv]

real example MapLayer that requires an APIKey that is stored in the indexdb.

[johnd0e]

real example MapLayer that requires an APIKey that is stored in the indexdb.

Good example.

1. Easiest way to handle it - wrap it into L.LayerGroup (empty in the beginning), and defer actual L.TileLayer adding (into that LayerGroup) till your request finished.
   BTW, there is general-purpose plugin with similar functionality, but it's easy to handle without it.

2. More special way would be implementing special L.TileLayerDeferred. But only if you have many such layers.

3. Or you can try (a) to make request ASAP (even outside of setup).
   (b) In setup just add L.TileLayer, but without apikey, and set promise/callback on request finish, that will merge apikey into layer's options.
   (c) In 99.9% cases you get apikey before actual tiles requests.

   In worst case some tiles may be broken.
   But, a @McBen already stated, you anyway should not put anything time-consuming in setup.
   So your promises-handling idea is obviously clever, but it may provoke plugins writers to introduce unacceptable delays.
   (Still, I am not saying that we reject it)

[johnd0e]

Some general thoughts:

• Until now we avoided promises, because of limiting to ES5.1 (see ES2015+? #47).
  So currently we have sort of 'callback hell' in some parts of core. That should be refactored at some (not so far) point, then we will return to this PR too.
• In 1 part of our api we already use jQuery-provided promises. It's obvious that we should stick to some single promises type.
  Sole reason of using jQuery promises - ES5.1 limitations.

But it is ridiculous to stick to ancient ES in 2019, so...

[johnd0e]

https://gist.github.com/Aradiv/978e3071c5203f6dedd25d7543d9aaa5

I see here wrong usage of iitcLoaded hook.
As you already stated, there could be something like this:

!window.iitcLoaded && addHook('iitcLoaded', hookFunc); || hookFunc();

But I agree that here promise will be much more appropriate that hook (we are considering this too, e.g. see #76 (comment)).

As for your gist sample, alternatively it could be fixed like this:

  setup : function(){
    let iitcLoaded = new Promise(resolve => {
      addHook("iitcLoaded", resolve);
    });
    let myrequest = new Promise(//......
    Promise.all([iitcLoaded, myrequest]).then(() => {
      window.plugin.mapTileOpenStreetMap.addLayer(); 
    });
  }

(myrequest may be started even before setup)

[johnd0e]

And finally (honestly, it's rather offtopic here, but anyway), speaking about reasonable api of your sample plugin: there is no need to expose any of internal functions in plugin's namespace, as (in your sample) no one will be able to gain from it.

So I would propose slightly different structure for it:

// use own namespace for plugin
let mapTileOpenStreetMap= {};
window.plugin.mapTileOpenStreetMap = mapTileOpenStreetMap;

mapTileOpenStreetMap.layers = { // public, possible to override
  'https://{s}.tile.openstreetmap.org/{z}/{x}/{y}.png': 'OpenStreetMap',
  'https://{s}.tile.openstreetmap.fr/hot/{z}/{x}/{y}.png': 'Humanitarian',
};

function addLayer() { // private

function setup() { // private