Log if slide index is not a number #8982
Conversation
| @@ -493,7 +493,7 @@ export class AmpSlideScroll extends BaseSlides { | |||
| */ | |||
| showSlideWhenReady_(value) { | |||
| const index = parseInt(value, 10); | |||
| if (isFinite(index)) { | |||
| if (isFinite(index) && index > 0 && index < this.noOfSlides_) { | |||
There was a problem hiding this comment.
Can't the index be 0?
Further: is this error comes from predefined use case, such as swiping or auto-advance/loop? Or from user-defined user action (on action). If on-action - this should probably be user().error() if from predefined behavior - it should probably be a dev.error.
| @@ -514,6 +514,7 @@ export class AmpSlideScroll extends BaseSlides { | |||
| */ | |||
| showSlide_(newIndex) { | |||
| const noOfSlides_ = this.noOfSlides_; | |||
| newIndex = dev().assertNumber(newIndex); | |||
There was a problem hiding this comment.
To clarify: dev.assertNumber is stripped in PROD binary. Is this what you want?
|
PTAL |
| const newSlideInView = this.slides_[newIndex]; | ||
|
|
||
| if (newSlideInView === undefined) { | ||
| dev.error( |
There was a problem hiding this comment.
It should be dev(), right?
| if (newSlideInView === undefined) { | ||
| dev.error( | ||
| TAG, | ||
| 'Accessing a non-existant slide at index: %s, noOfSlides: %s', |
There was a problem hiding this comment.
I don't think errors support %s format, unfortunately. It's also best to construct an error with a non-var message and instead pass arguments in Error.args map.
There was a problem hiding this comment.
Sorry about that i blindly converted from one method to another , fixed it now - also added a return; (since we are logging this.)
camelburrito
force-pushed
the
p1
branch
2 times, most recently
from
a7b339e
to
d502240
Compare
|
PTAL |
* Log if slide index is not a number * Review changes * Review changes * Fixing tests * lint
* Log if slide index is not a number * Review changes * Review changes * Fixing tests * lint
Investigation logging for #8920