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

amp-story-shopping JSON Validation I2I #36460

Closed
processprocess opened this issue Oct 21, 2021 · 16 comments
Closed

amp-story-shopping JSON Validation I2I #36460

processprocess opened this issue Oct 21, 2021 · 16 comments
Assignees
Labels
INTENT TO IMPLEMENT Proposes implementation of a significant new feature. https://bit.ly/amp-contribute-code Stale Inactive for one year or more WG: Caching: Triaged Initial triage from wg-caching complete. Remove label if new input required. WG: stories

Comments

@processprocess
Copy link
Contributor

processprocess commented Oct 21, 2021

Summary

JSON will be specified inline or fetched from a src attribute.
An inline configuration is required. If src is specified it will override the inline JSON.

To prevent CLS, the experience will render once the request resolves. If the request rejects, the experience will render using the inline config.

Data will be validated by the client at the component-level.
It is validated by the client since the config can be loaded via src.

If any required fields are missing or invalid, no templates will be rendered for the associated product (PDP, product tag, PLP) and a warning will be thrown in the console.

In the case that both src and inline JSON are specified, src will take precedence.

Optional fields that are invalid still will block rendering.
Optional fields that are missing will not block rendering.
Optional fields with required child attributes will block rendering if they are found to be invalid or missing.

isValidConfig = false; should not be set when an optional config field is present && validateRequired is not in the array. I'm adding in some additional checks for this.

JSON

{
   items: [
      {
         productId: "..." // Required. Keys to amp-story-shopping-tag nodes.
         productUrl: "...", // Required. String. Links to the product's website.
         productTitle: "...", // Required. String.
         productPrice: 100, // Required. Number.
         productPriceCurrency: "..." // Required. String. ISO 4217 currency code used to display the correct currency symbol.
         productImages: [ // Required. Array of objects.
            {
               url: "..." // Required. String.
               alt: "..." // Required. String.
            }
         ],
         productDetails: "...", // Required. String.
         aggregateRating: { // Optional. All sub fields are required if defined.
            "ratingValue": 4.4, // Required. Number.
            "reviewCount": 89, // Required. Number.
            "reviewUrl": // Required. String. Links to page where user can read reviews.
         },
         productBrand: "...", // Optional. String.
         productIcon: "...", // Optional. Links to an image. Defaults to a shopping bag icon.
         productTagText: "...", // Optional. String.
      },
      …
   ]
}

A validation config object will store a list of valid attributes. Each entry pairs to an array of validation functions to run against the value.

Validation and the validation config will be contained within the amp-story-shopping-config component.

Validation Config:

const productValidationConfig = {
   'productId': [validateRequired, validateStringLength, validateHTMLId], 
   'productBrand': [validateStringLength],
   'productIcon': [validateURL],
   …
}

Validation Scalability

In future versions of the component, additional fields may be added to render different templates.

For example, a “productDetails” page may be added to pick the size and color of a product. This validation pattern can be expanded to accommodate nested validation configs. In the case that a productDetails entry exists, the productDetailsValidationConfig can be run to check its contents.

Scaled JSON

{
   items: [
      {
         productUrl: "...", // Required. String. Links to the products website.
         productId: "..." // Required. Keys to amp-story-shopping-tag nodes.
         productBrand: "...", // Optional. String.
         productIcon: "...", // Optional. Links to an image. Defaults to a shopping bag icon.
         productTagText: "...", // Optional. String.
         productTitle: "...", // Required. String.
         productPrice: 100, // Required. Number.
         productPriceCurrency: "..." // Required. String. ISO 4217 currency code used to display the correct currency symbol.
         productImages: [ // Required. Must have at least one entry. Array of objects.
            {
               url: "..." // Required. String.
               altText: "..." // Required. String.
            }
         ],
         productDetails: "...", // Required. String.
         aggregateRating: { // Optional. All sub fields are required if defined.. 
            "ratingValue": 4.4, // Required. Number.
            "reviewCount": 89, // Required. Number.
            "reviewUrl": // Required. String. Links to page where user can read reviews.
         }
         productOptions: {
            size: "..." // Required. Array of strings.
            color: "..." // Required. Array of strings.
         }
      },
      …
   ]
}

Scaled Validation Config:

const productValidationConfig = {
    …
   'productOptions': [validateProductOptions]
}

const productOptionsValidationConfig = {
   'size': [validateRequired, validateStringEntries],
   'color': [validateStringEntries]
}

TODO

Investigate using JSON Schema.
Consider SWR (stale-while-revalidate) for caching src data.

Notifications

/cc @ampproject/wg-approvers

@calebcordry
Copy link
Member

I like @alanorozco 's idea of potentially using JSON Schema as a solution. We discussed a bit further offline and @alanorozco had some further insight into what this might look like that I will try to summarize:

The main potential downside of adding a validation library seems to be the bundle size increase. Looking at a few of the off the shelf options, they all seem too large.

size
35.1 kB https://bundlephobia.com/package/[email protected]
13.2 kB https://bundlephobia.com/package/@hyperjump/[email protected]
6.9 kB https://bundlephobia.com/package/[email protected]

As an alternative we could still use JSON Schema for the spec, but write our own lightweight interpreter for our codebase. This would allow us to have a standardized format that could also be validated server side with an off the shelf library where size is likely not a concern while also helping keep our bundle size increase manageable.

We may need to keep an eye on our interpreter to ensure it also does not become bloated, if this becomes a future problem @alanorozco has a few ideas on how we could have a transformer generate optimal code to help minimize bytes.

This may have a slightly higher up front cost but could be worth it for easier extensibility in the future, the ability to eventually share the validation logic across extensions, and having the validation spec easily available server side for all.

@newmuis
Copy link
Contributor

newmuis commented Jan 21, 2022

I missed this design review, but +1 to leaning into JSON schema. It's powerful and flexible enough to accommodate future requirements, and it's popular enough to have robust tooling support should we need it in the future.

Good question, though, re: bundle size.

@gmajoulet
Copy link
Contributor

As great as JSON schema, the draft PR linked here (#37474) shows that we can do something that'll scale in just ~100 lines, avoiding loading + using a [6.9, 35.1]kb library.

Approved

@banaag banaag added the WG: Caching: Triaged Initial triage from wg-caching complete. Remove label if new input required. label Feb 1, 2022
@swissspidy
Copy link
Contributor

Could a publicly hosted JSON schema file still be provided to help with tooling?

@gmajoulet
Copy link
Contributor

Totally, it'll be documented. I think @processprocess has been keeping this issue up to date with the latest API though.

@processprocess
Copy link
Contributor Author

processprocess commented Feb 22, 2022

Hey @swissspidy :) Just to clarify what would be helpful:
Hosting a JSON schema to spec the API and use for validation within tooling?

Pinging @jshamble and @calebcordry who may have ideas for how to implement this.
I think it would be great to use JSON Schema for the spec. The schema could be in a separate file so it could be pulled into tooling. Let's connect on how we can do that, it would be great for that to be a follow up to the validation PR. We will need to keep it in-sync with any config changes.

A note that we do not want the schema to be part of the bundle.

@swissspidy
Copy link
Contributor

Hosting a JSON schema to spec the API and use for validation within tooling?

Yes precisely 👍

@jshamble
Copy link
Contributor

jshamble commented Feb 24, 2022

I'll look into Hosting a JSON Schema after I get the Validation PR in.

@newmuis
Copy link
Contributor

newmuis commented Mar 1, 2022

Is the aggregateRating key really required here? What happens if I don't have ratings for the item?

@processprocess
Copy link
Contributor Author

processprocess commented Mar 1, 2022

Is the aggregateRating key really required here? What happens if I don't have ratings for the item?

Good point. If a product is new it won't have ratings yet.
To support new products aggregateRating needs to be optional. I'll update the documentation.

#37805 will allow the template to render without aggregateRating.

@newmuis
Copy link
Contributor

newmuis commented Mar 2, 2022

True, though I guess I also meant "what if, as a platform, I do not support ratings"?

@newmuis
Copy link
Contributor

newmuis commented Mar 2, 2022

Thanks @processprocess for #37805; @jshamble can you make sure we account for this in #37474?

@jshamble
Copy link
Contributor

jshamble commented Mar 2, 2022

@newmuis @processprocess informed me of this discussion yesterday and it was changed in #37474 as soon as I heard about it.

@swissspidy
Copy link
Contributor

Is my understanding correct that the productImages key is required, but it's not actually required to contain any images? There's no minContains in the schema or anything.

Asking because I just opened #38174 regarding that.

@jshamble
Copy link
Contributor

jshamble commented May 9, 2022

@swissspidy I think It should require at least one image, I'll update the schema to reflect this requirement.

@stale
Copy link

stale bot commented Jun 18, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale Inactive for one year or more label Jun 18, 2023
@ychsieh ychsieh closed this as completed Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
INTENT TO IMPLEMENT Proposes implementation of a significant new feature. https://bit.ly/amp-contribute-code Stale Inactive for one year or more WG: Caching: Triaged Initial triage from wg-caching complete. Remove label if new input required. WG: stories
Projects
None yet
Development

No branches or pull requests

8 participants