Add possibility to define custom layer properties

[sronveaux]

Hi team,

When new modules are added, we often have to add new properties on layers, for example, hoverable and hoverAttribute in the HoverController or columnMapping in the AttributeTable.
This is also true with custom modules a user could potentially add in its own app.

However (but correct me if I'm wrong) there is no easy way now to add custom layer properties through the config file as unknown properties are not taken into account (which is great by the way !).

If such attributes must be added now and we don't want to touch the code inside src, we have to add a function that re-parses the config file and add those attributes to the layers on the map or find other subtle ways to do it.

What I propose here is to add an optional global configuration property customLayerProperties which is an array that contains the names of properties a user could declare as usable inside the mapLayers.

Addition is very short, tests are included, the hardest is to explain its usage concisely inside the documentation.

Hope you'll find this simple addition useful!

Cheers

[fschmenger]

Hi Sebastien, a good addition.
Maybe you could move the handling of custom properties inside the LayerFactory, just adding those to getCommonLayerOptions

wegue/src/factory/Layer.js

Lines 89 to 105 in 63591aa

	getCommonLayerOptions (lConf) {
	return {
	lid: lConf.lid,
	isBaseLayer: lConf.isBaseLayer,
	previewImage: lConf.previewImage,
	displayInLayerList: lConf.displayInLayerList,
	supportsPermalink: lConf.supportsPermalink,
	extent: lConf.extent,
	visible: lConf.visible,
	opacity: lConf.opacity,
	opacityControl: lConf.opacityControl,
	zIndex: lConf.zIndex,
	confName: lConf.name,
	confAttributions: lConf.attributions,
	legend: lConf.legend,
	legendUrl: lConf.legendUrl,
	legendOptions: lConf.legendOptions

by doing
, ...lConf.customLayerProperties at the end should do the trick (untested).
More from me in a couple of weeks when I hopefully have a bit more time,
Felix

[fschmenger]

Just another idea: Would it be nicer declaring those custom properties without a wrapping customLayerProperties object, so they appear flat in the scope of the layer configuration object (like we also do for modules)? @chrismayer Any opinion on this?
However this involves some refactoring of the LayerFactory to pick the properties that are treated in a specific way first and then forward the remaining ones - as described above.

[sronveaux]

Hi @fschmenger,

Thanks for the fast reaction and good to hear some news from you !

What you propose is exactly in relation to what I thought at first and I agree 100% doing this would be better in the LayerFactory than patching after layer creation as I eventually did.

In fact, both solutions can work but result in different approaches at the beginning:

With your solution, each layer should define a customLayerProperties object containing what should be included. This means all layers can define their own distinct set of custom properties (not necessarily uniform from layer to layer) where my idea was to have the user pinpoint the properties which can potentially be associated to each and every layer. That's why I ended up with the customLayerProperties added in the root of the configuration object used to define which properties should be considered as, like you wrote, common layer options. After that, those properties can be added directly in the layer configuration object as all other properties from 'stock' modules. You can refer to the unit tests to see what it looks like in practice.

So on one side, you can define a custom object on each layer but no control is done on its content. On the other side, you define a list of properties to be considered as valid then layer definitions can be done as it's already done with all other modules.

However, using my approach, including those properties inside the LayerFactory is not possible without refactoring it in its entirety because for now, the LayerFactory is in fact a collection of functions with the main one being getInstance which takes a layer configuration as parameter but has no idea of the whole config file where customLayerProperties is defined. So applying your idea without refactoring everything would mean tweaking layer configuration objects and injecting a customLayerProperties object on them before calling getInstance then apply what you proposed... That's why I ended with the proposed implementation which is very short and doesn't imply any refactoring to existing code...

So two questions here I think should be discussed:

1. Should the customLayerProperties (or any name) be defined for each layer or in the root config?
2. Should the layer factory be entirely refactored and how based on answer to the first question?

Edit: Another option for question 1, obviously, is NOT to define a customLayerProperties at all then treat all unknown properties as custom ones as @fschmenger said in its latest comment. This means no control is done on what can and is injected inside the layer objects. That's another approach to be considered too...

[sronveaux]

Hi team,

Just thought again a little bit about all this, here's my personal view about this:

1. I'd personally stay with the customLayerProperties (or any name) array in the root configuration object used to define which properties can be added to the layers as I did in this first implementation. It's not 100% satisfactory but with this way of working, we keep the current behaviour where parameters unknown to the system are silently discarded. Users have to explicitly define the new parameters they want to use which gives them some responsibility when defining them and force them to double check properties definition to minimize, for example, typing errors...
2. In terms of refactoring, the only function of LayerFactory which is directly called from outside it is the getInstance one, which is called from the createLayers method of the Map component. We can pass a third parameter to it, either a customLayerPropertiesList or the full appConfig just in case it could be useful later and move the new addCustomLayerProperties function inside the LayerFactory then call it at the end of getInstance, just before returning the layer. That way, refactoring is minimal (a single function is changed) and custom layer properties are handled where it is logical to do it.

Just tell me what you think about it and give your personal opinions,

Sebastien

[fschmenger]

Hi Sebastien. My bad, sorry for misreading the code. I should have taken some time to check what it is doing before giving some recommendations. :)

I'm not sure whether it is a good feature to use a global customLayerPropertiesList as a sort of whitelist for allowed layer properties. E.g. for modules we're just silently forwarding every config property into the modules. So I wonder if we`re overcomplicating things here. But I leave the decision to @chrismayer.

Otherwise if we decide to use the customLayerPropertiesList approach, then I would pass it as an additional param to the Factory as you pointed out in 2)

Cheers,
Felix

[chrismayer]

Hi Sebastien and Felix, I personally like the dynamic behavior of JS and JSON at least for frontend development. So I really like the approach of the modules by just silently forwarding every config property. According to this and to keep things a bit harmonized in the app-config JSON and the later on interpretation of it I guess I would favor an approach without a "customLayerProperties".

Refactoring the LayerFactory is totally fine (in whatever way), especially in the current stage of the project, where we strive towards version 2.0.0.

I am not deciding, how to proceed with this. We should find a consent/compromise as a community. Further opinions? Or is someone willing to take over the implementation of a possible refactoring of the LayerFactory at all? If not we could postpone the decision. Merging as is, to have at least a possibility to extend the layer properties as an intermediate step? Unsure... please share your thoughts. Thanks in advance 🙏

[sronveaux]

Hi team,

As said, before, I don't think neither that the customLayerProperties is 100% satisfactory and I completely understand your point of view. For sure, with modules, the whole configuration object is forwarded but in this case, the module is written to make use of some properties. What is useless will stay in the configuration object and that's all. With layers, the underlying problem is that potential useless or damaging data will be assigned to the layer objects. It's not that useless properties will be bypassed as they are now (only needed properties are currently extracted of the config object when creating the layers), they will effectively be assigned to the underlying Openlayers layer objects in all cases...

However, if you are all O.K. with this, it's not a problem to remove it completely and "just" refactor the LayerFactory instead. I already thought a little bit about it, the main point which I didn't like is that we'll have to define some static properties name lists to keep track of the 'known' properties for each layer type (mainly because some of them are to be assigned to the Layer and some to the underlying Source). Just give me some time to remember all I've thought about and I'll come with a refactoring proposal... however, I would not merge as is as an intermediate step in that case as this would mean we'd introduce some new config parameters that will be removed in a (hopefully) not too distant future. If you really think this can be useful and want to merge current implementation, I would then personally remove all documentation about it or clearly indicate this is unstable and provisional...

Cheers,
Sébastien

[fschmenger]

Thanks @chrismayer and @sronveaux for the ongoing work and the useful input.
What would you think about the following implementation:

• LayerFactory is refactored to silently forward every configuration attribute without any requirement to define customLayerProperties.
• There`s a sort of "blacklist" in the LayerFactory which contains properties that correspond to OL internals - see https://openlayers.org/en/latest/apidoc/module-ol_layer_Layer-Layer.html (and similarly other layer types). If those appear in the layer configuration (and are not part of the Wegue documentation) it should result in a warning - or even error.

[sronveaux]

Hi team,

I made a quick test last Friday to keep refactoring as minimal as possible and here is how I thought this can be achieved:

In the LayerFactory object, getInstance(), which is the entry point can stay as is, then we can refactor all the createXXXLayer functions like this for example:

createOsmLayer (lConf) {
    const { crossOrigin, name, attributions, ...layerAttributes } = lConf;

    const layer = new TileLayer({
      source: new OsmSource({
        crossOrigin
      }),
      confName: name,
      confAttributions: attributions,
      ...layerAttributes
    });

    return layer;
  },

The idea is to extract the properties needed for the layer type (+name and attributions I will explain afterwards) and assign all the remaining ones to the layer directly.
There is no real need to have the getCommonLayerOptions function anymore as they will automatically be taken into account in the remaining properties.

If we take a close look at the whole file, all the parameters have the same name inside the config file than what they are assigned to afterwards... except name and attributions which are respectively mapped to confName and confAttributions. That's why they are extracted on their own...

Here's another example for completeness:

  createTileWmsLayer (lConf) {
    const { url, layers, params, serverType, tileGrid, projection, crossOrigin, hoverable, hoverAttribute, hoverOverlay, name, attributions, ...layerAttributes } = lConf;

    // apply additional HTTP params
    const effectiveParams = { LAYERS: layers };
    ObjectUtil.mergeDeep(effectiveParams, params);

    const layer = new TileLayer({
      source: new TileWmsSource({
        url,
        params: effectiveParams,
        serverType,
        tileGrid,
        projection,
        crossOrigin,
        hoverable,
        hoverAttribute,
        hoverOverlay
      }),
      confName: name,
      confAttributions: attributions,
      ...layerAttributes
    });

    return layer;
  },

With current implementation, undefined properties are not added inside the layer objects as they are for now inside getCommonLayerOptions. This has no impact in practice as accessing an unknown property in JavaScript returns undefined. However, if we want to be explicit, we can also call getCommonLayerOptions and refactor it to consume the needed properties and return the remaining ones next to a commonLayerOptions object.

This can also be "upgraded" to take into account the idea of a "blacklist" given by @fschmenger, which I really think is great, however, what if the user do want to redefine some OpenLayers attributes as it is the case for now with opacity, visible or extent for example ? That's exactly why I ended adding the customLayerProperties at first to force users to be explicit about their intent...

So, just tell me what you think about this draft example of refactoring and please give your opinion about those two points:

• Should we keep getCommonLayerOptions to explicitly create potential undefined values ?
• Should some kind of "blacklist" be implemented to filter the parameters before assigning them to the layers ?

Thanks in advance,
Cheers,
Sébastien

[sronveaux]

Apologies team, typed too fast, hit the tab key and finished publishing an unfinished comment and closing the PR which was not what I wanted for sure...

[fschmenger]

Hi Sebastien, great you got working on this again.
Regarding the getCommonLayerOptions: I'm not sure if OL layer properties should be forwarded implicitly or explicitly (for clarity) . Maybe @chrismayer has an opinion on this one?! Either way you should

• make sure that attributes treated in a special way by LayerFactory are removed from layerAttributes - to not add them twice.
• Regarding confName and confAttributions you could check if we can just deprecate them and change their config name to name and attributions.
• If new OL layer properties are now implicitly supported by this change (you were mentioning extent, opacity etc.) they should be at least listed explicitly in the documentation. It's nice to support new properties but you have to make sure they don't interfere with other concepts and what we have in code elsewhere.

I think a blacklist is probably required, as nothing good will happen if you e.g. override map.

Cheers,
Felix

[chrismayer]

Thank you @sronveaux for your ongoing work and your input @fschmenger. I like the proposal of @sronveaux in general. Here are some thoughts:

I think a blacklist is probably required, as nothing good will happen if you e.g. override map.

If I got it right "the black list" should contain layer properties, which we want to avoid being set by the application config JSON, right? So these should be mainly props with non-serializable property values, which cannot be formalized in the JSON. So possible candidates for the black list are props awaiting OpenLayers object instances or functions. One could argue that if a user sets the map property of a layer e.g. to a double value, nothing good will happen, but surely expectable. So for my understanding the black list is "only" a kind of convenience- and security-layer for users. Means more work on our side to maintain those properties, but maybe worth the effort. I go with whatever you agree, with or without the black list.

Should we keep getCommonLayerOptions to explicitly create potential undefined values ?

• I don't think that a getCommonLayerOptions is necessary as long as we only support non-transformed values, which could be formalized in the JSON file. And the others, like OpenLayers object instances or functions are potential candidates for the black list, see above. All others should be possibly applyable.

Regarding confName and confAttributions you could check if we can just deprecate them and change their config name to name and attributions.

As far as I remember the config options are name and attributions and are only transformed in the LayerFactory to confName and confAttributions.

[fschmenger]

As far as I remember the config options are name and attributions and are only transformed in the LayerFactory to confName and confAttributions.

You`re right, sorry for not checking the code. :) This was required for localization.

[sronveaux]

Hi team and thanks for the fast comments on this one. I totally agree with both of you on these which unfortunately leads me to some more questions and thinkings...

At first, good to hear @chrismayer that the general proposal is O.K. for you. I'll continue in that direction then.

To summarize for the rest:

Should we keep getCommonLayerOptions to explicitly create potential undefined values ?

Perfect to note that explicit forwarding is not needed. However, I think I'll keep such a function to extract the ones to be renamed (name and attributions for now) to keep the code more DRY if it's O.K. for both of you.
As mentioned in the comments, this renaming is somewhat necessary for localization and should be kept if we don't want to overcomplicate things...

So for my understanding the black list is "only" a kind of convenience- and security-layer for users. Means more work on our side to maintain those properties, but maybe worth the effort. I go with whatever you agree, with or without the black list.

As stated earlier, I really like the idea of the "black list" but the more I think about it, the more I think the effort to maintain it correctly will be bigger than expected. As stated by @chrismayer, to be useful, it should contain functions and other non-serializable properties of all classes linked to a layer type which means a lot (almost 30 items just for the BaseLayer object). I just fear that this list will be very long and that the majority of it will be near to 100% sure nobody will have an idea to use (who would use a parameter called getUseInterimTilesOnError ?).
Implementing this "black list" will force us to verify it every time we upgrade OpenLayers. Is it a good idea ? Or should it be restricted to a defined smaller list (but in that case which names to include and will it be really robust and useful ?) ? If you think it won't be annoying in the future, I'll implement it though...

Thanks again for the ideas and opinions on this one and hope we'll find the best option all together shortly !

Cheers,
Sébastien

[fschmenger]

Hi Sebastien, sorry for being quiet for so long.

If a blacklist is to cumbersome to maintain, then maybe we have been trying to pull things up from the wrong side.
At a second thought:
Why not be explicit about whats assigned to the top-level options of the OL layer constructor and then forward everything that's left over generically into options.properties- see https://openlayers.org/en/latest/apidoc/module-ol_layer_Base-BaseLayer.html -
I haven`t looked at the OL internals, but if everything is handled well, then there should be no chance that we override anything.

So e.g. something along these lines:

const layer = new XXXLayer({
   ....this.getCommonLayerOptions(lConf), /* explicit forwarding of common options, e.g. those defined on the BaseLayer */
   ....this.getSpecificLayerOptions(lConf) /* explicit forwarding of options specific on the layer type like currently handled in the createXXXLayer methods, . e.g. source, tileGrid, projection etc. */
   properties: forwardRemainingProperties(lConf) /* every lConf property not handled above goes into options.properties (NOT into options directly) */
 })

Do you think this would be more futureproof /easier to handle?

[sronveaux]

Hi Felix, no problem and thanks for coming back to it,

I'm myself a bit out of time for now and will concentrate on your comments on the other PRs first then your request for review before coming back to this one...

The idea seems interesting for sure and would be way easier to maintain in the end. I have to take some time to refresh everything on this one as the elapsed time made me forget all the details... which is a good thing for this one! I'd like to get back to OL internals with "fresh eyes" now before considering all the possibilities we have... as soon as I'll have a little time for this, I'll post some new thoughts about it!

Cheers,
Sébastien