search

ampproject / wg-components

Responsible for AMP components, the overall health of AMP Pages, analytics features, and integrations with partner technologies. Facilitator: @alanorozco
Creative Commons Attribution 4.0 International
11 stars 20 forks source link

Discussion: <Migrate GPay Widgets from AOG Fork to Public AMP > #9

Closed kunal-rp closed 3 years ago

kunal-rp commented 5 years ago

Background

The payments team currently is supporting two amp widgets for publishers to use for payment options; the google payment button and the inline payment widget.

The Problem

Up until now, these widgets have resided on a seperate fork of AMP that the Actions on Google(AOG) team has been maintaining. With the constant changes to both these components and the public amp repo, there is now a significant overhead associated with the maintanence of this amp fork.

Thus, the AOG and payments team is looking into merging the payments widgets onto the public repo to remove the need for the maintenance.

aghassemi commented 5 years ago

@ampproject/wg-ui-and-a11y @jeffjose @rudygalfi @choumx

As the working group that will own Payments in AMP going forward, we need to collectively make a consensus-based decision on a proposal from @kunal-rp regarding amp-payment-google-* components.

As the moderator, I will hold on expressing my own opinion for now and instead drive the discussion.

The decision we need to make is whether to: "merge amp-payment-google-* components into AMP core

A few points to start the discussion:

A few questions to start the discussion:

nainar commented 5 years ago

My concerns with having an ACTION specific component:

ianba commented 5 years ago

These extensions already exist and are in use, and merging with the public git doesn't change that fact. However since these are limited to the ACTIONS validator rule, their usage will continue to be limited to Actions use cases.

On the contrary, keeping these extensions in the private fork just ensures continued development with minimal feedback from the broader AMP team.

jeffjose commented 5 years ago

@ianba, @kunal-rp - I have a quick clarification on the implementation -

My understanding is that these extensions will only work with AoG viewer, since it loads pay.js in the background and the extensions offloads most of the heavy lifting over to the pay.js since the extensions themselves are not PCI compliant.

Is that accurate?

ianba commented 5 years ago

Yep, that's correct, the extensions talk to the AoG viewer, which delegates to pay.js

aghassemi commented 5 years ago

@ianba So then I assume there is not path to make this work in non AoG pages as it stands.

aghassemi commented 5 years ago

I think we have enough information here to make a decision.

My vote would be no since it only works for a very particular viewer. I think maintaining a fork of AMP is necessary for such cases where components are platform specific. When payments in AMP is ready and if they work for AoG's use-cases, then the fork can go away.

@ampproject/wg-ui-and-a11y @jeffjose please share your thoughts.

nainar commented 5 years ago

+1 to @aghassemi's assessment.

dreamofabear commented 5 years ago

Thanks for the thoughtful responses.

My vote would be no since it only works for a very particular viewer.

The original intent was actually to have broad support. From https://github.com/ampproject/amphtml/issues/15201:

The goal is to eventually make these extensions work on both Google and non-Google surfaces.

However, broad support could muddle messaging with the upcoming AMP Pay offering. So the proposed compromise is to offer these components in a limited manner -- they only work with viewer support on the ACTIONS spec.

This restricted usage means that cost (in terms of user confusion) of merging these extensions should be minimal. E.g. we can keep them unlisted on ampproject.org, and anyone who stumbles across them in GitHub won't be able to get them to work anyways.

On the other hand, consider the cost of not merging these extensions:

Overall I think this is an exceptional case due to timing (GPay, AMP Pay, team changes, open governance), so we shouldn't worry about this setting an unsavory precedent.

That said, AMP has a history of iterating on component strategy e.g. amp-skimlinks vs. amp-rewrite-links, and amp-video-iframe vs. amp-brightcove et al. Also, migrating usages of amp-brightcove to amp-video-iframe is basically impossible, but the same won't be true for migrating GPay widgets into AMP Pay due to the restricted usage set and close contact with partners.

aghassemi commented 5 years ago

My concerns are less about the API and more about the fact that amp-payment-google-* is currently a shell component and the engine behind it ( pay.js ) is in the Google AoG viewer, not the component itself.

This is very different than amp-skimlinks and amp-video-iframe examples. We don't have precedence in AMP for component that are just a format and their real implementation is somewhere else.

I actually think we will not endup with amp-pay as an API and most likely we end up with components such as amp-payment-google-*, amp-stripe, etc.. as part of Payments in AMP project so I am not really worried about API.

I see two paths forward:

  1. GPay team, @jeffjose and @cathyxz work together and agree to prioritize GPay as the first implementation in AMP Payments project. GPay team makes the existing components work in all AMP environments working with @jeffjose and @cathyxz to ensure use-cases, compliance, and technical implementation are all aligned. (@cathyxz has already started researching Payments in AMP and it is in our roadmap for Q2). This option is effectively: "implement https://github.com/ampproject/amphtml/issues/15201"
  2. We get buy-in from TSC that the concept of shell components is something we want to have, rename to amp-actions-gpay-* and merge as-is with a different WorkingGroup (amp-actions-wg?) owning it.
jeffjose commented 5 years ago

I wanna chime in here and say that at this moment merging amp-payment-google* would complicate the work we're undertaking with amp-pay. We'd end up with two similarly named components that would cause developer confusion going forward. There's nothing in the name amp-payment-google-* that says it is specific for AoG usecase.

That said, the good news here is I think there's a way for amp-payment-google* to benefit from the amp-pay work scheduled for Q2. At a high level, the latter is bringing payments (including pay.js) to open web, and with a small refactor amp-payment-google can use amp-pay behind the scenes to deliver the same experience to ACTIONS.

Timeline wise by mid-Q2 we should have a design ready after which we can evaluate how amp-payment-google* can use pay.js coming from amp-pay instead of AoG viewer.

dreamofabear commented 5 years ago

We don't have precedence in AMP for component that are just a format and their real implementation is somewhere else.

Fair, though we do have components that facilitate interaction with viewers e.g. amp-viewer-integration. What if we renamed the components to amp-viewer-gpay-[inline|button] instead?

aghassemi commented 5 years ago

Fair, though we do have components that facilitate interaction with viewers e.g. amp-viewer-integration. What if we renamed the components to amp-viewer-gpay-[inline|button] instead?

That's essentially option 2 from https://github.com/ampproject/wg-ui-and-a11y/issues/9#issuecomment-479556925 which I am personally fine with (would still prefer amp-actions-gpay-* as the name, but that's just details). I don't think this WG needs to approve that approach. It's a fine short-term solution IMO anyway.

dreamofabear commented 5 years ago

SGTM. Synced with the Actions team and the new proposal is to name the extensions amp-viewer-gpay-[inline|button] with stewardship under wg-runtime (or perhaps wg-viewers).