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

Fix for resource#resolveDeferredsWhenWithinViewports_ to use parseFloat #16238

Conversation

keithwrightbos
Copy link
Contributor

Fix for #16218

delete this.withViewportDeferreds_[viewportInt];
const viewportNum = dev().assertNumber(parseFloat(keys[i], 10));
if (this.isWithinViewportRatio(viewportNum, viewportRatio)) {
this.withViewportDeferreds_[viewportNum].resolve();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the fix was to check if this.withViewportDeferreds_[viewportNum] is defined.

if (this.isWithinViewportRatio(viewportInt, viewportRatio)) {
this.withViewportDeferreds_[viewportInt].resolve();
delete this.withViewportDeferreds_[viewportInt];
const viewportNum = dev().assertNumber(parseFloat(keys[i], 10));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parseFloat(1.25, 10) = 1.25, which leads to this.withViewportDeferreds_[1.25]. I think we should keep using parseInt

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm, just learnt that the key here is a string, where we are expecting this.withViewportDeferreds_['1.25']

if (this.isWithinViewportRatio(viewportInt, viewportRatio)) {
this.withViewportDeferreds_[viewportInt].resolve();
delete this.withViewportDeferreds_[viewportInt];
const viewportNum = dev().assertNumber(parseFloat(keys[i], 10));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm, just learnt that the key here is a string, where we are expecting this.withViewportDeferreds_['1.25']

// used as numbers in isWithinViewportRatio.
const viewportNum = dev().assertNumber(parseFloat(keys[i]));
if (this.isWithinViewportRatio(viewportNum, viewportRatio)) {
this.withViewportDeferreds_[viewportNum].resolve();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use keys[i]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I do so, I get ERROR - restricted index type
found : string
required: number

Casting via /** @type {number} */(keys[i]) doesn't help

@keithwrightbos keithwrightbos merged commit b9280d4 into ampproject:master Jun 22, 2018
@keithwrightbos keithwrightbos deleted the resource_withinViewport_parseFloat branch June 22, 2018 02:28
zhouyx pushed a commit that referenced this pull request Jun 22, 2018
…at (#16238)

* initial commit

* add comment

* Fix type check error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants