error if invalid 'before' argument is passed to Map#addLayer #5401
Conversation
mollymerp
force-pushed
the
invalid-before
branch
from
2371f4e
to
429c8c9
Compare
src/style/style.js
Outdated
| @@ -568,7 +568,10 @@ class Style extends Evented { | |||
|
|
|||
| layer.setEventedParent(this, {layer: {id: id}}); | |||
|
|
|||
| const index = before ? this._order.indexOf(before) : this._order.length; | |||
| const beforeIndex = before ? this._order.indexOf(before) : 0; | |||
There was a problem hiding this comment.
@mollymerp is 0 the top or bottom of the stack? Hitting map.style._order in my console, the first layer in the array is visually under all other layers on the map.
There was a problem hiding this comment.
I suppose this is misleading – beforeIndex isn't actually used unless before exists so the 0 value never gets assigned to index. I made it 0 instead of null to appease flow but I could make this clearer.
mollymerp
force-pushed
the
invalid-before
branch
from
429c8c9
to
73b2a80
Compare
src/style/style.js
Outdated
|
|
||
| const beforeIndex = before ? this._order.indexOf(before) : -1; | ||
| const index = before && beforeIndex !== -1 ? beforeIndex : this._order.length; | ||
| if (before && beforeIndex === -1) util.warnOnce(`Layer with id "${before}" does not exist on this map. Adding layer to the top of the stack.`); |
There was a problem hiding this comment.
Let's report this via this.fire('error', ...) for consistency with non-existent layer IDs in moveLayer, removeLayer, etc.
test/unit/style/style.test.js
Outdated
| }] | ||
| })); | ||
| const layer = {id: 'c', type: 'background'}; | ||
| t.stub(console, 'error'); |
There was a problem hiding this comment.
Bind a handler for the error event rather than stubbing console.error (see the subsequent test for example).
src/style/style.js
Outdated
|
|
||
| const beforeIndex = before ? this._order.indexOf(before) : -1; | ||
| const index = before && beforeIndex !== -1 ? beforeIndex : this._order.length; | ||
| if (before && beforeIndex === -1) this.fire('error', { |
There was a problem hiding this comment.
Instead of adding to the top of the stack, return early without making changes in the case of an error.
src/style/style.js
Outdated
| @@ -568,7 +568,12 @@ class Style extends Evented { | |||
|
|
|||
| layer.setEventedParent(this, {layer: {id: id}}); | |||
|
|
|||
| const index = before ? this._order.indexOf(before) : this._order.length; | |||
|
|
|||
| const beforeIndex = before ? this._order.indexOf(before) : -1; | |||
There was a problem hiding this comment.
this._order.length can never be -1, so the conditionals can be simplified:
const index = before ? this._order.indexOf(before) : this._order.length;
if (index === -1) {
// error
}
this._order.splice(index, ...);
There was a problem hiding this comment.
ah good point! I still had to use if (before && index === -1) for flow to allow me to include before in the error message.
mollymerp
force-pushed
the
invalid-before
branch
2 times, most recently
from
dec81e9
to
8c022d3
Compare
mollymerp
force-pushed
the
invalid-before
branch
from
8c022d3
to
6dcc1ab
Compare
mollymerp
changed the title
fix #3868
Launch Checklist