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-app-banner:1.0] Ensure Bento utilities are document-scoped #37465

Closed
scottrippey opened this issue Jan 24, 2022 · 5 comments
Closed

Comments

@scottrippey
Copy link
Contributor

As discussed in ampproject/bento#5, in order for Bento components to work correctly in an AMP document, they should not use the global document, but instead should reference the web-component's ownerDocument.
These 2 PRs are currently using utilities with a globally-scoped document, and should be improved to use the proper doc scope:

@scottrippey
Copy link
Contributor Author

scottrippey commented Jan 28, 2022

Before working on this, I would like to establish a better understanding of the requirements.

  • Find/write documentation describing the "document scope" issue
  • Locate/create an example of the problem, using actual code. Preferably a unit test, or test page that can be debugged.

Once this is done, we can establish a pattern to ensure the correct document scope is respected.

@scottrippey
Copy link
Contributor Author

scottrippey commented Jan 31, 2022

For reference, here's a branch that contains some FIE examples: 1bd121aa66

This commit also contains an example of the "Service Injection" approach, which is done via AmpComponent.useContexts and the Context API.

@scottrippey
Copy link
Contributor Author

Some thoughts on how to solve this document-scope problem:

Service Injection

We use AMP services when in AMP mode, and use a fallback "standalone" service when in Bento mode.
Example:

// amp-component.js
AmpComponent.useContexts = [UrlServiceContextProp];

// component.js
function BentoComponent() {
  const urlService = useContext(UrlServiceContext); // in Amp mode, returns the UrlService, otherwise returns a fallback
}

Pros:

  • The Preact framework already provides an easy way to inject services, using contexts. See useContexts in this example 1bd121aa66
    Cons:
  • This ties the Bento component to an AMP service, making it harder to separate the two
  • This is hard to enforce via static type checking
  • The Bento "fallback" service will have to mimic the AMP service's API, even if the API is needlessly async or has a lot of methods

@scottrippey
Copy link
Contributor Author

scottrippey commented Jan 31, 2022

Creating document-scoped utilities

Instead of creating a dependency between Bento and AMP services, we could ensure our new "utilities" can handle document-scoping themselves. The component can retrieve the ownerDocument from a core-supplied Context, and all utilities would use that document as a scope:

function BentoComponent({ }) {
  // in Bento and AMP mode, an OwnerDocumentProvider will supply the web component's `ownerDocument`.
  // in React mode, the `ownerDocument` will default to global `document`
  const ownerDocument = useContext(OwnerDocumentContext);  

  const parsed = urlUtils.forDoc(ownerDocument).parse('foo/bar');
}

Pros:

  • Removes coupling between AMP and Bento
  • Only a single implementation of the "service/util" will be used ... reduces complexity, service implementations won't "drift"
  • Semantic and simple

@stale
Copy link

stale bot commented Mar 19, 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 Mar 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

3 participants