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

🐛Change validator to let amp-access and amp-subscriptions coexist #27280

Merged

Conversation

kushal
Copy link
Contributor

@kushal kushal commented Mar 18, 2020

Although the valdiator error is useful for pushing users to avoid errors, in practice amp-access and amp-subscriptions appear to coexist fine. This change is needed to allow publishers using amp-subscriptions to also use amp-access-scroll to let Scroll customers avoid ads.

@amp-owners-bot
Copy link

Hey @ampproject/wg-caching, these files were changed:

extensions/amp-access/validator-amp-access.protoascii
extensions/amp-subscriptions/0.1/test/validator-amp-subscriptions-errors.out
extensions/amp-subscriptions/validator-amp-subscriptions.protoascii

@honeybadgerdontcare
Copy link
Contributor

/cc @chenshay

@jpettitt
Copy link
Contributor

LGTM from a wg-access-subscriptions standpoint - not ideal but after VC with @kushal I think this is the least worst solution to the conflict between amp-access-scroll and amp-subscriptions.

@honeybadgerdontcare not sure what the validator test fail is, the error message is not very enlightening, can you give any pointers on how to fix?

@honeybadgerdontcare
Copy link
Contributor

I'll look at this more closely later but this is the test that is failing. Essentially it's trying to ensure satisfies and requires matches up. https://github.com/ampproject/amphtml/blob/master/validator/engine/validator_test.js#L1662

Although the valdiator error is useful for pushing users to
avoid errors, in practice amp-access and amp-subscriptions
appear to coexist fine. This change is needed to allow
amp-subscriptions users to also use amp-access-scroll to
let Scroll customers avoid ads.
@kushal kushal force-pushed the access-subscriptions-validator branch from cc6910d to e37ffae Compare March 19, 2020 18:17
@kushal
Copy link
Contributor Author

kushal commented Mar 19, 2020

I'll look at this more closely later but this is the test that is failing. Essentially it's trying to ensure satisfies and requires matches up. https://github.com/ampproject/amphtml/blob/master/validator/engine/validator_test.js#L1662

Thanks! I tried removing the relevant satisfies and requires but it still appears to fail.

@jpettitt jpettitt merged commit 8022487 into ampproject:master Mar 19, 2020
twintwox pushed a commit to twintwox/amphtml that referenced this pull request Mar 24, 2020
Although the valdiator error is useful for pushing users to
avoid errors, in practice amp-access and amp-subscriptions
appear to coexist fine. This change is needed to allow
amp-subscriptions users to also use amp-access-scroll to
let Scroll customers avoid ads.
honeybadgerdontcare added a commit to honeybadgerdontcare/amphtml that referenced this pull request Mar 25, 2020
honeybadgerdontcare added a commit to honeybadgerdontcare/amphtml that referenced this pull request Mar 25, 2020
honeybadgerdontcare added a commit that referenced this pull request Mar 25, 2020
* cl/302030563 Revision bump for #27280

* cl/302033641 Remove trailing whitespace from protoascii

* cl/302035592 Revision bump for #27159

* cl/302061262 github commit msg missing or malformed

* revert

* revert

* revert

* revert

* revert

* trying to kickstart travis

Co-authored-by: Greg Grothaus <greggrothaus@google.com>
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