add Map#isSourceLoaded #3691 #4033
Conversation
There was a problem hiding this comment.
Thanks for the PR @stepankuzmin! Couple of minor comments inline.
| * @returns {boolean} A Boolean indicating whether the source is loaded. | ||
| */ | ||
| isSourceLoaded(id) { | ||
| const source = this.style.sourceCaches[id]; |
There was a problem hiding this comment.
Note that it's possible for Map to be constructed without a style, which would cause this line to blow up. Something like const source = this.style && this.style.sourceCaches[id]; would work.
| error: new Error(`There is no source with ID '${id}'`) | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
While it's true that the contributing guidelines say to use error events, I think throwing an error might be more appropriate here, because we know immediately(synchronously) that there's an error.
When, as in this case, synchronous error reporting is possible, I think it's preferable, as it's generally easier to diagnose/fix. (E.g.: because of the return;, a user of this method might not even notice the error, since something like if (map.isSourceLoaded(id)) { ... } would still work without blowing up.)
…or instead of firing event mapbox#3691
| isSourceLoaded(id) { | ||
| const source = this.style && this.style.sourceCaches[id]; | ||
| if (source === undefined) { | ||
| throw new Error(`There is no source with ID '${id}'`); |
There was a problem hiding this comment.
Per #3208, we have a convention that we emit an error event rather than throw an error.
There was a problem hiding this comment.
@lucaswoj ah, shoot -- this was a mistake on my part in this comment. I'd seen the guideline in CONTRIBUTING but didn't take it as strict/universal. My bad.
|
@stepankuzmin Oof, my apologies re: #4033 (review) -- you had it right the first time 😞 |
|
No problem, @anandthakker. I've fixed that :) |
|
Thanks @stepankuzmin ! |
Add
Map#isSourceLoadedaccording to #3691