Remove support to multiple consent instance #20089
Conversation
zhouyx
force-pushed
the
disable-multi
branch
from
d876202
to
a0ed951
Compare
| this.consentInstanceIdDepr_ = consentInstanceId; | ||
| this.init_(); | ||
| } | ||
|
|
||
| /** | ||
| * Register the policy instance | ||
| * Example policy config format: | ||
| * { | ||
| * "waitFor": { | ||
| * "consentABC": [], // Can't support array now. | ||
| * // All items will be treated as an empty array |
There was a problem hiding this comment.
Should we move around these comments now that we removed the bottom consent? 😄
There was a problem hiding this comment.
Yes I wanted to completely remove the support to waitFor. However, I can't do that now because it may break some of the existing usage. I'll create a follow up PR to support the new format and migrate later.
| Promise.all(initPromises).then(() => { | ||
| instance.startTimeout(this.ampdoc_.win); | ||
| /** | ||
| * |
| } | ||
|
|
||
| /** | ||
| * |
| this.consentInstanceIdDepr_ = consentInstanceId; | ||
| this.init_(); | ||
| } | ||
|
|
||
| /** | ||
| * Register the policy instance | ||
| * Example policy config format: | ||
| * { | ||
| * "waitFor": { | ||
| * "consentABC": [], // Can't support array now. |
There was a problem hiding this comment.
Since we now only have one consent, does this format still make sense? 🤔
There was a problem hiding this comment.
Replied here #20089 (comment)
There was a problem hiding this comment.
My apologies for mentioning the Unblock vs. Enable, we half-committed, and after discussing offline, let's just go back to unblock 😂
Other than that, everything LGTM to me. As you mentioned, I looked over the tests, I think they all look good (syntactically), and I couldn't think of more cases to test. Looks all well tested 😮 unless there was something specific you wanted me to look at? Let me know 😄
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Wait for policy instance to be ready. | ||
| * Wait for policy instance to be registered. | ||
| * @param {string} policyId | ||
| * @return {!Promise} | ||
| */ | ||
| whenPolicyInstanceReady_(policyId) { |
There was a problem hiding this comment.
nit: Can we renamed this is registered as well? 😄
There was a problem hiding this comment.
renamed to whenPolicyInstanceRegistered_
| } | ||
| return this.whenPolicyInstanceReady_(policyId).then(() => { | ||
| return this.instances_[policyId].getReadyPromise().then(() => { | ||
| return this.instances_[policyId].shouldBlock(); | ||
| return this.instances_[policyId].shouldEnable(); |
There was a problem hiding this comment.
As we discussed offline, let's just keep this as unblock, my apologies 😢
There was a problem hiding this comment.
haha it's a great point! and I enjoy reading the article a lot! It's just blocking/unblocking the build of other AMP components sound better to me in this very specific use case.
| * Caller need to make sure that policy status has resolved | ||
| * @return {boolean} | ||
| */ | ||
| shouldBlock() { | ||
| shouldEnable() { |
There was a problem hiding this comment.
As we discussed offline, let's just keep this as unblock, my apologies 😢
There was a problem hiding this comment.
renamed back to shouldUnblock
|
|
||
| describes.realWin('ConsentStateManager', {amp: 1}, env => { | ||
|
|
There was a problem hiding this comment.
nit: too many newlines 😄
* consent policy do not support multi instances * sync and nit * address comment1 * address comment2 * remove new lines
* consent policy do not support multi instances * sync and nit * address comment1 * address comment2 * remove new lines
As agreed, we decide to completely remove support to multiple consent instance/prompt. All such use case could be satisfied using CMP approach.