-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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: "protocol adapters" for client side data massaging. #26474
Comments
There is one thing about this proposal thats been bugging me. The inversion of control from Edge cases:
We might be better served by allowing the For the same use case above: <html>
<head>...</head>
<body>
<button on="click:amp-list.refresh()"> Refresh data </button>
<amp-list src="fn:animals-script.fetchAnimalData">...</amp-list>
<script type="text/plain" target="amp-script" id="animals-script">
function fetchAnimalData() {
return fetch('url-with-data.json')
.then(res => res.json())
.then(data => {
let next = data.next ? `http://url-with-data-${data.next}` : undefined;
return { items: data.animals, next };
});
}
</script>
</body>
</html> Note: no use case should ever need to pass data into these functions since they can call Also note: this solution for protocol adapters would potentially create a clean solution for #25684 |
Your second example is what I was personally thinking about and hoping for:
Generally speaking, I've been hoping that So, devs would get to use all the features of If this works, you could get closer to my dream for #25684 - as you have pointed out! /cc @alankent for his opinion |
In other words, we keep the simplicity of our interactive components, but we let developers inject a little logic of their own in nice, native JavaScript. |
primary question for design review
|
I am not sure I follow "should AMP components or amp-script have control of the flow?" So trying to rephrase my thoughts. I think what Ben said expresses my thoughts too. If I need to use amp-script to massage the request and response, I don't want to do a major page redesign thinking about different control flows. AMP pages for me should be declarative (as much as possible anyway). So just because I need a protocol adapter, I should not start needing amp-script wrappers around parts of the page (if possible). I don't understand all the refresh etc edge conditions. But I think about it more simply. Consider a product page that has 2 selectors for shirt size and color. Updating either one should cause the page to refresh. I imagine AMP state would be used to remember the current values. Clicking a button should update the AMP state to the new selection. If there was no protocol adapter, that would trigger an API call and page render. That is the goal - update state and amp-bind causes the amp-list to refresh. The only extra part to the mix is to say that the update to amp-state instead of causing amp-list to go fetch a So reloading a page should add like what a page would do without amp-script (just using amp-list and |
If I understand the question correctly, giving |
@alankent, I'll write out the shirts example (reminds me of ampcamp!) through the lens of the two proposals. (1) amp-script w/ most of the control <html>
<head>...</head>
<body>
<amp-script script="store-script">
<button id="shirt-size-button"> rotate shirt size</button>
<button id="shirt-color-button"> rotate shirt color </button>
<amp-list src="amp-state:shirtData" [src]="shirtData">...</amp-list>
</amp-script>
<script type="text/plain" target="amp-script" id="store-script">
function fetchShirtData() {
const size = AMP.getState().selectedSize;
const color = AMP.getState().selectedColor;
fetch(`/api?size=${size}&color=${color}`)
.then(res => res.json())
.then(transform)
.then(data => AMP.setState({ shirtData: data }));
}
document.getElementById('shirt-size-button').addEventListener('click', () => {
AMP.setState({selectedSize: ... }).then(fetchShirtData);
});
document.getElementById('shirt-color-button').addEventListener('click', () => {
AMP.setState({selectedColor: ... }).then(fetchShirtData);
});
fetchShirtData(); // first load
</script>
</body>
</html> (2) amp-list w/ most of the control <html>
<head>...</head>
<body>
<button id="shirt-size-button" on="tap:AMP.setState(...),items.refresh()"> rotate shirt size</button>
<button id="shirt-color-button" on="tap:AMP.setState(...),items.refresh()"> rotate shirt color </button>
<amp-list id="items" src="fn:store-script.fetchShirtData">...</amp-list>
<amp-script script="store-script"></amp-script>
<script type="text/plain" target="amp-script" id="store-script">
function fetchShirtData() {
const size = AMP.getState().selectedSize;
const color = AMP.getState().selectedColor;
return fetch(`/api?size=${size}&color=${color}`)
.then(res => res.json())
.then(transform)
}
</script>
</body>
</html> My take is that something more like (2) is significantly more preferable. |
I agree that (2) is more preferable... preferabler? I would even go further. I imagined that AMP would perform the <amp-list id="items" src="'/api/fetchShirtData?size=' + products.size + '&color=' + products.color" fn="store-script"> ... </amp-list>
<amp-script script="store-script"></amp-script>
<script type="text/plain" target="amp-script" id="store-script">
function massageShirtData(rawData) {
let transformedData = ... ;
return transformedData;
}
</script> What do you think? |
@morsssss: I'm okay with that idea, it definitely simplifies the 80% use-case. My reservations would be:
|
Hi @samouri - no detailed comment from me just now, but I think you are understanding the issues correctly and heading in the right direction. I definitely want to add special headers etc (e.g. generate a SOAP request with special authentication headers, or generate 3 separate API requests and merge the results). |
I can't recall my requests exactly, but I suppose these suggestions depend on whether AMP creates the xhr or the worker. If the worker performs the xhr, you can leave it up to the publisher to encode url params as necessary and specify headers (notably, the |
The consensus in the design review was to pursue option (2) instead of option (1). |
hooray! (@alankent , I'm now getting hives at the memory of making SOAP services) |
Closing as this feature has launched in #29689 |
summary
The objective is to solve the protocol adapters use case. We want to allow developers to perform arbitrary client side transformations to an api response before handing it to an amp component, essentially trading UX for DX.
Since
amp-script
is already capable of performing xhr requests, data manipulations, and can call AMP.setState, it is close to being fully capable of servicing this use-case.assumptions
basic amp-list w/ refresh example
motivation
amp-list
, they create proxy servers. This is a poor DX for something that could easily be solved via a sprinkling of JS.caveats
amp-script
has a nontrivial initialization cost. If after we perform measurements we deem it too high to release this feature, there are opportunities we can pursue for speeding up init, especially for this use case.reset-on-refresh
behavior (solvable by exposing amp actions within amp-script).The text was updated successfully, but these errors were encountered: