-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Bento: amp-lightbox-gallery:1.0 must be unique on the document #35104
Conversation
@@ -41,15 +41,33 @@ const DEFAULT_GROUP = 'default'; | |||
/** @const {string} */ | |||
const DEFAULT_CAROUSEL_PREFIX = 'carousel'; | |||
|
|||
/** @const {number} */ | |||
let count = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this tracks that only 1 instance exists, why can it be a boolean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, we can replace the boolean value on unmount.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to use a count instead of a boolean because there is no way to differentiate with the boolean alone whether the component unmounting is the original instance or a duplicate instance. We would have to store an additional state variable to discern whether or not to flip the bit on unmount. Otherwise, imagine this scenario:
- Mount first instance -
hasInstance = true
- Mount second instance -
(hasInstance === true) => unmount
- Unmount second instance -
hasInstance = false
- Mount third instance - incorrectly allowed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we check hasInstance === true
when mounting, then the second instance shouldn't mount, correct? Anyways, we might be overengineering this, I'll approve the current approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second instance would mount, realize it's a duplicate, and immediately unmount. But in doing so, it would flip hasInstance
back to false
-- given that the simplified logic sequence wouldn't behave differently than if it was the first instance getting unmounted for some unrelated reason (in which case a subsequence instance should be able to mount, build, and etc.)
More concretely, it would have had to be:
let hasInstance = false;
mountCallback() {
if (hasInstance) {
/** unmount */
- }
+ } else {
hasInstance = true;
+ this.primaryInstance = true;
+ }
}
unmountCallback() {
+ if (this.primaryInstance) {
hasInstance = false;
+ }
}
Counting was just a way to avoid having to do that :P
@alanorozco and I also discussed blocking inside the constructor
, in which case duplicates would never get to mount/unmount and we could use the boolean without the additions above. However this caused obscure errors (this hasn't been initalized - super() hasn't been called
), and unit testing that component creation short circuits without having access to buildInternal
wasn't straight forward. For that reason, and so I could also surface a helpful warning instead of a weird error, I preferred to use the mountCallback
approach.
Uniqueness was prior enforced in AMP mode via
installLightboxGallery
and validation rules. For Bento mode, neither of these measures will be in place, so we must enforce it by counting mounted instances, only allowing the first to initialize, and detaching the rest with a warning. Follow up from #35073 and partial for #32759