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

I2I: Allow amp-list to have its json data sourced from amp-state #26009

Closed
samouri opened this issue Dec 12, 2019 · 17 comments
Closed

I2I: Allow amp-list to have its json data sourced from amp-state #26009

samouri opened this issue Dec 12, 2019 · 17 comments
Labels
Component: amp-list INTENT TO IMPLEMENT Proposes implementation of a significant new feature. https://bit.ly/amp-contribute-code WG: runtime

Comments

@samouri
Copy link
Member

samouri commented Dec 12, 2019

summary

We've had multiple requests to support having amp-list source its data from an amp-state (1). In order to make this happen, there are multiple problems we must solve:

  1. Syntax for pointing an amp-list to a key of amp-state . Note that this does not necessarilly need to correspond with a literal <amp-state> in the HTML.
  2. A way of informing the amp-list of amp-state metadata. Specifically, a piece of state being requested may be in one of four states: NotRequested | Loaded | Errored | Refreshing. This is so that amp-list can decide to render a loading placeholder, error message, or the data.
  3. Rendering data from initial load: amp does not allow for AMP.setState to cause component rerenders unless there was a user gesture. We'd still want an amp-list to render on pageload though even without user interaction.

What follows are my proposals for solving each of these issues. I'm new to amp and won't take any harsh criticisms personally, so please feel free to be critical of these ideas!

syntax proposal

In #15647, there were a few good proposals for syntax, and I'd vote we stick with this one:

<amp-list src="amp-state:json.path">

What I like about this syntax is that it is abundantly clear to the reader that the component is pointed at amp-state without being too verbose. One of the alternatives was to use src=#json.path. With that suggestion I'd fear that a developer could think it must point to an id on the page.

metadata proposal

This is the part where I go off the deep end. In order for amp-list to render properly it needs state metadata, where the state always is the result of a fetch. When I think fetch I think Promise. What if you could call AMP.setState with a promise? That would allow amp-state to seamlessly and separately keep track of state metadata. amp-list could then depend on that key.

For example:

<amp-script script="example"></amp-script>
<script type="text/plain" target="amp-script" id="example">
      const promise = fetch(url).then(processData);
      AMP.setState({ promised: promise });
</script>

<amp-list src="amp-state:promised.subkey">
  <template type="amp-mustache">
    <div>{{ title }}</div>
  </template>
</amp-list>

What would this mean? From the developer's perspective, it is expected that the type of the promise is Promise<!JsonObject>. Once the promise resolves, it will be deep-merged at the specified key just like a regular setState call. The developer shouldn't need to worry about how the state metadata is being tracked.

Initial load proposal

On initial load, we could create a small time window for an amp-list to subscribe to amp-state and await the registration of a promise for its state json.path.

@samouri samouri added the INTENT TO IMPLEMENT Proposes implementation of a significant new feature. https://bit.ly/amp-contribute-code label Dec 12, 2019
@samouri
Copy link
Member Author

samouri commented Dec 16, 2019

Use cases to think through

initial load

  1. initial load of an amp-list from a static json amp-state.
  2. initial load of an amp-list from a url-based amp-state.
  3. initial load of an amp-list from an amp-script sourced state.

refreshing (incl option to have a loading indicator)

  1. refreshing an amp-list that is pointed to a url-based amp-state.
  2. refreshing an amp-list that is pointed to a key in amp-state and set by a bind expression.
  3. refreshing an amp-list that is pointed to a key in amp-state and set by amp-script.

@samouri
Copy link
Member Author

samouri commented Dec 16, 2019

updated proposal (after conversations with chmoux and jridgewell)

  • Syntax: same as above
  • Metadata: Instead of having amp-bind accept promises within a setState call, we maintain that people can only call setState with bind expressions/json. amp-script can still hold on to metadata regarding its state, and it can be automatically managed in the url-based case. In order to also have metadata available in the amp-script case, we can expose actions for the amp-script to imperatively set the state back to loading. Note that this requires us to implement the FR allowing amp-script to trigger component actions.
  • Initial load: this is only a question in the amp-list based on an amp-state set by amp-script. I think we need to give amp-list some kind of liberal grace period for its first render.

This now means that if one points an amp-list to amp-state and they want to have the amp-list reevalute they'll need to:

  1. Somehow update the state. Examples of how to do this include:
    1. triggering the amp-state.refresh action.
    2. calling AMP.setState within amp-script or a bind expression.
  2. After the state has finished updating, trigger an amp-list.refresh()

@mattwomple
Copy link
Member

@samouri I mentioned in #26034 that I'd like to work on #25719 and @choumx suggested I touch base with you on this I2I. At face value, I think the feature I would like to implement (explained in more detail here: #25988) is different than what you're aiming to complete, but I do see areas of overlap in that we could share a means of delivering modified content to amp-mustache for final rendering. Is there a chance we could chat about goals and timeline soon? I'm in PST time zone.

@samouri
Copy link
Member Author

samouri commented Dec 17, 2019

Is there a chance we could chat about goals and timeline soon? I'm in PST time zone.

Sure! I'll ping you on our Slack channel.

@samouri
Copy link
Member Author

samouri commented Dec 18, 2019

@mattwomple and I just met to discuss overlap between this I2I and Protocol Adapters. It does look like having amp-list sourced by an amp-script based state would also solve his use case. But there are a few open questions that lead me to think that we should still rethink parts of this I2I:

  1. If using amp-script, how much overhead are we adding because of worker-dom even though the protocol adapter use case doesn't require it. Could we add a flag or some way of indicating a dom-less amp-script?
  2. This I2I is trying to create a comprehensive solution for flexibly sourcing data based on amp-state. If we tried to specifically create a solution for amp-list protocol adapters, we could do something more tailored for that experience like matt demonstrated here.
  3. Are there any special security considerations that amp-script takes in to account? Could we create a smaller Worker-based approach for Protocol Adapters considering they don't need to modify dom.

Matt, feel free to add in anything I missed!

@dreamofabear
Copy link

Agree on (1). There's also a similar issue of amp-bind's overhead (which amp-state is bundled with).

Assuming we can make performance acceptable, I'd prefer a more generic mechanism over a one-off.

@mattwomple
Copy link
Member

mattwomple commented Dec 18, 2019

Thanks for posting the notes @samouri. @choumx do you have any initial feedback on # 3? The overhead of directly using Worker() seems quite a bit smaller than loading components amp-script and amp-bind when they might not be needed. On the other hand, if amp-script has a lot of safety mechanisms built-in that would need to be reproduced in a direct invocation of a Worker, then I can see its worth.

@dreamofabear
Copy link

Hm, one nice feature amp-script supports is inline scripts which require CSP-like script hash, which is probably useful if the transformer code is small.

Re: (2), a nice property of <amp-list src="ampstate:foo.bar"> is it supports data sources other than JSON endpoints e.g. WebSocket.

With regard to size, I think decoupling amp-state from amp-bind should be doable and probably desirable anyways.

@samouri
Copy link
Member Author

samouri commented Dec 18, 2019

Using the default amphtml gulp webserver, I did some performance testing. I created two versions of the same page, one directly using amp-list with a json source, and the other using amp-script to hit the same endpoint and then call .setState() with a child amp-list whose bound src points to that location in state. All to simulate the overhead of using amp-bind and amp-script. See: https://gist.github.com/samouri/66d5fa5abe8c0296222f84f0b0ebf233

assumptions

  • a special made worker just for protocol adapters would have an unnoticable performance hit compared compared to regular usage of amp-list.
  • that manually testing against a local gulp webserver has external validity

results
I'm calling the direct amp-list version "light" and the amp-script version "heavy".

  • light: 200-300ms
  • heavy: 700ms-1.75ms

@mattwomple
Copy link
Member

mattwomple commented Dec 19, 2019

There's one other consideration we should keep in mind. Right now, amp-list supports "fetch and forget", i.e. data is fetched, rendered by mustache, then eventually garbage collected. This is beneficial when the data sources are large. As I understand it, if the new pattern would be fetch, set amp state, render amp state with amp-list, then that data stays in memory. It should only scale by the number of amp-lists taking advantage of this feature (i.e. refreshing an amp-list will overwrite data, not append), but I think it's worth noting.

EDIT: Thanks for posting the performance numbers @samouri . At the very least, that's further evidence that amp-script should be optimized for this application.

@mattwomple
Copy link
Member

Another consideration to keep in mind: amp-form success/failure templates could also benefit from protocol adapters, so it would be great if work could be written with flexibility in mind (use across multiple components).

@samouri
Copy link
Member Author

samouri commented Dec 24, 2019

I spent much of today measuring init performance for amp-bind and amp-script. I believe my earlier "light" numbers had to have been a mistake since I cannot repro them now. I'm guessing the CPU Slowdown 6x wasn't taking effect since today I've run into a bug a few times where the UI and actual cpu slowdown were out of sync.

new numbers

  • light (just an amp-list): ~1.2s
  • heavy (amp-script and amp-bind): ~1.8s

opportunities for closing the gap

  1. Move up the request for the worker script files. ww.js for amp-bind and amp-script-worker-0.1 for amp-script. Today we don't even initiate those network requests until ~1s into page-load, and with a 6x slowdown I was seeing them take ~400ms to complete each (but they happened in parallel).
  2. Is it possible to have the workers initialize in parallel with v0 (or at least much earlier on if we detect that we'll need it)? If we do that, we may be able to remove the vast majority of their overhead (without having to split out amp-state etc.)

@dreamofabear
Copy link

amp-script-worker-0.1.js can likely just be inlined into amp-script.js as a string const. We already do this for CSS e.g. see import {CSS} from ... in extensions.

For amp-bind, there's actually no functional dependency on ww.js for this use case (e.g. no expression parsing) so decoupling "AMP state" from amp-bind should bypass it. Though preloading is probably a good idea.

@morsssss
Copy link
Contributor

A note: I do like the amp-state: protocol, since it looks very clear and unambiguous! However, about the comment above:

One of the alternatives was to use src=#json.path. With that suggestion I'd fear that a developer could think it must point to an id on the page.

As the humble suggester of the # idea: doesn't in fact always point to an id on the page? We name each <amp-state> variable by giving it an id.

@samouri
Copy link
Member Author

samouri commented Jan 14, 2020

doesn't in fact always point to an id on the page? We name each variable by giving it an id.

Each <amp-state> element will surely have an id. And for the purposes of being able to SSR initial values for amp-list, I'd expect <amp-state> elements to exist every time.

This stops holding true in the protocol adapters use-case. Users may call AMP.setState in an <amp-script>, and they may set keys which have no corresponding <amp-state> elements in the HTML.

For example:

<html><body>
  <amp-list src="amp-state:animals">... </amp-list>
  <amp-script script="local-script"></amp-script>
  <script id="local-script" type="text/plain" target="amp-script">
     AMP.setState({ animals: ['cat', 'dog', 'okapi'] });
  </script>
</body>
</html>

Please let me know if you think I'm missing something and/or if you still have a strong preference for #json.path!

@morsssss
Copy link
Contributor

I see your point! So, essentially, we're decoupling state variable names from HTML IDs.

I'm excited to see your <amp-script> use case there too :)

@samouri samouri changed the title I2I: Allow amp-list to have its json data sourced from amp-state. I2I: Allow amp-list to render from amp-state initially Jan 24, 2020
@samouri samouri changed the title I2I: Allow amp-list to render from amp-state initially I2I: Allow amp-list to have its json data sourced from amp-state Jan 24, 2020
@samouri
Copy link
Member Author

samouri commented Jan 24, 2020

There was some good discussion here. I'm going to close this issue though in favor of multiple more specific I2Is (one just for initialization, and another for the more general protocol adapters use-case)

@samouri samouri closed this as completed Jan 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: amp-list INTENT TO IMPLEMENT Proposes implementation of a significant new feature. https://bit.ly/amp-contribute-code WG: runtime
Projects
None yet
Development

No branches or pull requests

5 participants