fix duplicate layer test, enforce layer id uniqueness #6147
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -521,6 +521,11 @@ class Style extends Evented { | |||
|
|
|||
| const id = layerObject.id; | |||
|
|
|||
| if (this.getLayer(id)) { | |||
| this.fire('error', {error: new Error(`Layer with id "${id}" already exists on this map`)}); | |||
There was a problem hiding this comment.
Map.addSource will throw an error when attempting to add a source with a duplicate ID. Is there a reason to fire an error event instead of throwing for addLayer ?
There was a problem hiding this comment.
Firing an event is correct; see #3879.
There was a problem hiding this comment.
We made a decision awhile back to emit errors across the board instead of throwing #3879 (comment)
There was a problem hiding this comment.
whoops didn't reload and see john's comment 😬
jfirebaugh
reviewed
Feb 13, 2018
test/unit/style/style.test.js
Outdated
| @@ -992,20 +992,19 @@ test('Style#addLayer', (t) => { | |||
| }); | |||
|
|
|||
| t.test('emits error on duplicates', (t) => { | |||
| t.stub(console, 'error'); | |||
There was a problem hiding this comment.
hmm I guess not! left over from earlier debugging I think. removed!
jfirebaugh
approved these changes
Feb 13, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fix #5989
the test that was supposed to verify this was erroneously passing due to an extra
t.end()and validation in validate_layer doesn't work because we're passing inarrayIndex = -1so it never enters the loop to check duplicate ids.mapbox-gl-js/src/style-spec/validate/validate_layer.js
Lines 27 to 32 in e077b78
Launch Checklist