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

isSourceLoaded returns error instead of false #9430

Closed
nsteffens opened this issue Mar 17, 2020 · 6 comments
Closed

isSourceLoaded returns error instead of false #9430

nsteffens opened this issue Mar 17, 2020 · 6 comments

Comments

@nsteffens
Copy link

Hi there,

I'm using the mapbox-gl-js framework to display a polygon a map. The setup is quite simple, for licensing reasons i'm not allowed to publish the whole code.

See this code snippet:

if (map.isSourceLoaded('isochrone')) { map.removeLayer('isochroneLayer'); map.removeSource('isochrone'); }

This somewhat works fine, however, isSourceLoaded is returning an error if the source is not loaded:

evented.js:140 Error: There is no source with ID 'isochrone'
    at r.isSourceLoaded (map.js:1385)
    at <internal-code>.js:157

I'd expect the function to simply return false instead of an error. Is this a bug or expected behavior?

@stevage
Copy link
Contributor

stevage commented Apr 7, 2020

"Loaded" in this context means "the data for the source has been successfully retrieved", not "the source has been added to the map object". I think it's behaving as expected.

FWIW, I find the behaviour of throwing errors when trying to remove layers/sources that don't exist a bit annoying. My utility library mapbox-gl-utils suppresses them. (It also automatically removes layers from sources).

@nsteffens
Copy link
Author

Thanks for your explanation @stevage, I think I now understand how the method is actually meant. In the documentation, it simply says:

Returns a Boolean indicating whether the source is loaded.

I think i was not aware that a difference between sources and layers exist. It's difficult to say what to do, but form my point of view there's three possibilities:

  • Implement some type of error handling and just return false
  • Implement a hasLayer() method that eases checking if a layer is actually shown. (e.g. as in hasImage())
  • Stick with the current behaviour and guide people to use the getLayer() method (and check for an undefined value) - or what would be the more appropriate way?

I agree on your view regarding the annoyance of errors being thrown in this case. For now, this is still fine for me since the application it was used in was more a proof-of-concept.

@stevage
Copy link
Contributor

stevage commented Apr 7, 2020

It is true that this (and other similar states and events) are unclearly worded, and this can be a real pain point. See also #6707 and #6314.

Implement a hasLayer() method that eases checking if a layer is actually shown.

There is .getLayer(). But determining if a layer is "shown" is pretty complex, depending on what you mean by "shown". There are several states a layer can be in where it has been added to the map but nothing is visible to the user. (No geometry within view, style properties like 0 opacity, filters that match nothing, source-layer that matches nothing, etc).

@ryanhamley
Copy link
Contributor

If all you want to do is determine if a layer is visible within the current viewport map.queryRenderedFeatures(null, {layers: ['myLayer']}) is probably your best bet. But if what you're looking for is "is this layer renderable under any condition", that's a lot more nuanced as @stevage points out.

I think the error handling issue is valid and we can think about whether or not we should fix that. We're currently researching and thinking about our error handling so any examples of pain points are helpful.

If you need more assistance specifically with detecting a "loaded" layer, an example would help provide more insight @nsteffens. Even if you can't use the specific implementation from your app, a minimal, analogous example would help.

@nsteffens
Copy link
Author

nsteffens commented Apr 10, 2020

Thanks to both of you, let me do a bit of explanation of my use-case to provide some more Information. They map I'm creating is, after some internal calculations, fetching a custom geojson polygon from an internal API. This is added as a source with the id mySource. Now, when some settings are done in the UI, I need to clear this polygon from the map. If the user did not chose to show the mySource before, I'd have an error in the console.

Using the methods you mentioned above, I've checked the following on a blank map:

 // Will throw and return undefined: 
console.log(map.isSourceLoaded('mySource'))

// Will throw and return an empty Array:
console.log(map.queryRenderedFeatures(null, {layers: ['myLayer']}))

// Will _not_ throw and return undefined:
console.log(map.getLayer('myLayer'))`

To me, it seems getLayer is most suitable for this as I just have to check that it's not undefined.

In general, I basically do not need assistance on this. Due to this code only being used in an internal prototype, it's not that big of a problem. I just wanted to see if this "throw an error where I'd expect an boolean"-issue was known. Thanks for adding it to the master ticket! IMO this issue can be closed.

@asheemmamoowala
Copy link
Contributor

@nsteffens You can guard your call to isSourceLoaded with a call to getSource which will return undefined if there is not a source with the specified ID in the loaded style.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants