apply() and promises

[fleur]

Hello,

So it looks like the apply() function doesn't return a promise, only the applyStyle() does. Is there a particular reason for this? Is it somehow infeasible? I'd like to know if there's a reason it can't be done before heading down the rabbit hole...

thanks, fleur

[ahocevar]

apply() was meant to be simple, and was designed to return an OpenLayers map instance. If we'd return a promise, when should it be resolved? When all layers have been added to the OpenLayers map? When all layers have sources? When all layers have loaded source metadata? When the map has completed rendering all data for the first time? There does not seem to be a straightforward answer.

What would be your use case, or when would you consider a promise returned by apply() resolved?

[fleur]

I would consider the apply() promise resolved when all promises created by the package are resolved. To me, it seems straightforward that the package would expose all promises created by it.

From what I can tell so far, this would mean resolution when all layers have sources by any means, which yes, would mean that the promise may block until the user manually adds sources. Maybe this would cause too many support issues with users not understanding the possibility of blocking?

I'm trying to use OpenLayers 5 with React. Reconciling two different event management paths is tricky, or at least annoying. When I add ol-mapbox-style to the mix, using apply() on an existing map, there's no hook for me to tell React to re-render, and a promise returned from apply() seemed an obvious solution.

What about an intermediate function like applyToExistingMap() which could return a promise, and then apply() could call it and just not return the promise, to preserve the existing API. According to StackOverflow, if there's no reference to the promise, it'll get garbage collected, even if still pending.

BTW, ol-mapbox-style is awesome, and exactly what I need. Thank you! I'm hoping to be able to contribute back any enhancements I end up needing to make, so I am happy to take direction.

[ahocevar]

Your suggestion to resolve the promise when all promises created by the package are resolved makes sense. Regarding API, apply() could have 2 flavors:

1. When called with an ol/Map instance as 1st argument, it returns a promise.
2. When called with a string or HTMLElement for a map container as 1st argument, it returns the ol/Map instance it created.

This would still be a breaking change, but not sacrifice simplicity for users that call apply() with a map container reference.

If you're willing to work in that direction, that would be a welcome improvement.

[fleur]

I'm starting to write more specific tests for applyStyle() to get a better feel for the codebase. The onChange() method in applyStyle() is written to only apply styles to VectorLayer and VectorTileLayer or fail silently. It's odd because the style variable is declared undefined, and checked in onChange(), but there's nothing else in the function, or anywhere in index.js that sets it other then inside onChange() . If I reject instead of silently ignore, tests for handling raster sources and layers start failing.

Is this an oversight or intentional? Is there a reason it should silently fail, like it doesn't matter for some reason, and isn't really an error? I have basically no experience with map stuff and the geologist I'm doing this with is in another timezone with 3 kids, and information transfer between us isn't very good, so apologies if this is a map-related rank amateur question.

https://github.com/boundlessgeo/ol-mapbox-style/blob/a6ddc765b0415d212966c00cae8490378d5b981c/index.js#L121

    var spriteScale, spriteData, spriteImageUrl, style;
    function onChange() {
      if (!style && (!glStyle.sprite || spriteData)) {
        if (layer instanceof VectorLayer || layer instanceof VectorTileLayer) {
          style = applyStyleFunction(layer, glStyle, source, resolutions, spriteData, spriteImageUrl, getFonts);
        }
        resolve();
      } else if (style) {
        layer.setStyle(style);
      }
    }

[ahocevar]

Thanks for digging into this. The confusion comes from the fact that the applyStyle function is only meant to be used with Vector and VectorTile layers (as stated in the documentation).

The promise should be immediately rejected (right at the beginning of the function) when applyStyle is called with a layer that is not Vector or VectorTile.

[fleur]

Ok. Adding that makes these existing tests fail: https://github.com/boundlessgeo/ol-mapbox-style/blob/a6ddc765b0415d212966c00cae8490378d5b981c/tests/test.js#L71 https://github.com/boundlessgeo/ol-mapbox-style/blob/a6ddc765b0415d212966c00cae8490378d5b981c/tests/test.js#L136 https://github.com/boundlessgeo/ol-mapbox-style/blob/a6ddc765b0415d212966c00cae8490378d5b981c/tests/test.js#L186 I'll operate on the assumption that applyStyle() just shouldn't be called for raster layers in the first place, since they don't take styles anyway.

[ahocevar]

I'll operate on the assumption that applyStyle() just shouldn't be called for raster layers in the first place, since they don't take styles anyway.

Correct. Thanks!