Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
MichaelLangbein opened this issue Apr 21, 2022 · 2 comments
Labels
dependent depends on, blocked by other Issue enhancement feat: (New feature or request)

Comments

@MichaelLangbein
Copy link
Collaborator

MichaelLangbein commented Apr 21, 2022

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 MichaelLangbein added the enhancement feat: (New feature or request) label Apr 21, 2022
@boeckMt
Copy link
Member

boeckMt commented Apr 21, 2022

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 #74

FYI @voinSR @Stef-Firestone @lucas-angermann

@boeckMt
Copy link
Member

boeckMt commented Aug 26, 2022

Depends on: #74

@boeckMt boeckMt added the dependent depends on, blocked by other Issue label Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependent depends on, blocked by other Issue enhancement feat: (New feature or request)
Projects
None yet
Development

No branches or pull requests

2 participants