ukis-frontend-libraries icon indicating copy to clipboard operation
ukis-frontend-libraries copied to clipboard

Make certain at runtime that layers added to LayersService are actually instances of `Layer`

Open MichaelLangbein opened this issue 2 years ago • 2 comments

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 of Layer
  • 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 at updateLayer, delivers more immediate feedback to the user that the passed data does not contain all the information that Ukis requires.
  • 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-Layerobjects into actual instances of Layer 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.

MichaelLangbein avatar Apr 21 '22 07:04 MichaelLangbein

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

boeckMt avatar Apr 21 '22 10:04 boeckMt

Depends on: https://github.com/dlr-eoc/ukis-frontend-libraries/issues/74

boeckMt avatar Aug 26 '22 11:08 boeckMt