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

♻️ Remerge: Resources/Mutator service refactor #26638

Merged
merged 1 commit into from
Feb 12, 2020

Conversation

powerivq
Copy link
Contributor

@powerivq powerivq commented Feb 5, 2020

This PR would merge back the PR reverted for causing amp-auto-lightbox to fail. Since .getResources() call caused type check not be able to find out the type mismatch, I have again searched for all references to this method and found no other use of it.

@powerivq powerivq changed the title ♻️WIP Remerge: Resources/Mutator service refactor ♻️ Remerge: Resources/Mutator service refactor Feb 7, 2020
@powerivq
Copy link
Contributor Author

powerivq commented Feb 7, 2020

Once you are back and able. This PR is ready to be merged. @choumx @estherkim

@dreamofabear
Copy link

Does this PR contain any changes other than a roll-forward of #26479?

Since .getResources() call caused type check not be able to find out the type mismatch

Do we know why this was and did we fix the root cause?

@powerivq
Copy link
Contributor Author

powerivq commented Feb 11, 2020

@choumx It does not contain anything else.

Do we know why this was and did we fix the root cause?

The reason is that element does not have type declaration. We have changed them to use Services.mutatorForDoc which guarantees correct type inference, so this should not happen.

@dreamofabear
Copy link

dreamofabear commented Feb 11, 2020

The reason is that element does not have type declaration.

So this.element in base-element.js has a type of *?

Oh I see, the element used in amp-auto-lightbox.js didn't have the right type.

To catch these kinds of bugs in the future, we'd need a warning system for underspecified types. Please file an issue for this and reference it in your cherry-pick post-mortem (#26483).

@powerivq
Copy link
Contributor Author

@choumx make sense! created an issue.

@powerivq powerivq merged commit 27fccda into ampproject:master Feb 12, 2020
@powerivq powerivq deleted the mutator-ref-rr branch February 12, 2020 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants