ukis-frontend-libraries
ukis-frontend-libraries copied to clipboard
Make certain at runtime that layers added to LayersService are actually instances of `Layer`
Description
Consider this case:
const layer1 = new RasterLayer(paras);
const layer2 = { ... layer1 }; // This is a common mistake. What user should have done here is `new RasterLayer(layer1);`
layersService.add(layer1);
layersService.add(layer2); // no error here
layer2.visible = !layer2.visible;
layersService.updateLayer(layer2); // error here
This causes an error:
layer with id: <....> you want to update not in storeItems!
This error is thrown because updateLayer
calls isInLayergroups
which checks if the passed layer is an instance of Layer
if (layergroup instanceof Layer || layergroup instanceof LayerGroup)
Since layer2
was created using object-spread, it does not pass this test.
- What is missing?
- Check that all data that is passed to
addLayer
is actually an instance ofLayer
- Check that all data that is passed to
- What is it useful for?
- Copying objects using object-spread is very common, especially in state-management-solutions like ngrx. Throwing an error early, that is, at
addLayer
and not just atupdateLayer
, delivers more immediate feedback to the user that the passed data does not contain all the information that Ukis requires.
- Copying objects using object-spread is very common, especially in state-management-solutions like ngrx. Throwing an error early, that is, at
- Does this change impact existing behaviors? If so how?
- This should not impact functioning code, but might throw errors where others have made that same mistake.
Relevant Package
This feature request is for @dlr-eoc/services-layers
Describe alternatives you've considered
- One could quietly transform non-
Layer
objects into actual instances ofLayer
in the background. But that would make Ukis harder to understand and encourage passing badly formatted data. - Alternatively one could not check with
instanceof
but rather ducktyping by only checking if the immediately required attributes are present. But that would either lead to yet more checks or to more runtime-errors.
Creating instances and Shallow-cloning objets is a different thing which should not be mistaken.
But yes we could check this on add Layer, and give hint like ... is not an instance of Layer
Also in my VS Code I get the hint
Argument of type '{ type: TRasterLayertype; url: string; subdomains?: string[]; params?: IRasterLayerParams; tileSize?: number; name: string; id: string; opacity: number; visible: boolean; ... 20 more ...; cssClass?: string; }' is not assignable to parameter of type 'Layer'. Property 'time' is missing in type '{ type: TRasterLayertype; url: string; subdomains?: string[]; params?: IRasterLayerParams; tileSize?: number; name: string; id: string; opacity: number; visible: boolean; ... 20 more ...; cssClass?: string; }' but required in type 'Layer'.
So I can't add the layer.
Feel free to do a PR :)
This could be somewhere in isInLayergroups
because this function is called before add Layer or Group.
public isInLayergroups(layergroup: Layer | LayerGroup | string, groups?: Array<Layer | LayerGroup>): boolean{
...
if (layergroup instanceof Layer || layergroup instanceof LayerGroup) {
id = layergroup.id;
} else {
id = layergroup;
}
...
}
I can't remember why we allowed to add strings here... If we remove it we could use the else for this check.
We could also use this opportunity to do https://github.com/dlr-eoc/ukis-frontend-libraries/issues/74
FYI @voinSR @Stef-Firestone @lucas-angermann
Depends on: https://github.com/dlr-eoc/ukis-frontend-libraries/issues/74