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.

Sign up for GitHub

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

Jump to bottom

✨ [amp-list] Introduce "amp-state:" as a usable protocol for the src attribute. #26284

Merged

samouri merged 27 commits into ampproject:master from samouri:point-amp-state

Jan 28, 2020

Member

samouri commented Jan 9, 2020 •

edited

Loading

Summary
Implements #15647.

Allows an amp-list to initialize off of an SSRed amp-state. Initially I had been worried that this would delay the rendering of amp-list until amp-bind had finished initialization (ironically this could be slower than the client side fetch would have been). Turns out though, that amp-state parsing and getState are not blocked on amp-bind's initializationPromise, so all is well.

I've gated it on a feature flag, so nobody should be able to use it yet. In a followup PR I'll run performance measurements, create documentation and remove the flag.

cc @jridgewell / @choumx

googlebot added the cla: yes label

lgtm-com bot commented Jan 9, 2020

This pull request introduces 1 alert when merging dedd3fd into 5bfb0e5 - view on LGTM.com

new alerts:

1 for Missing 'this' qualifier

lgtm-com bot commented Jan 9, 2020

This pull request introduces 1 alert when merging 6778b02 into b338300 - view on LGTM.com

new alerts:

1 for Missing 'this' qualifier

samouri force-pushed the point-amp-state branch from 72f74be to 2a33705 Compare

January 10, 2020 22:31

Member Author

samouri commented Jan 11, 2020 •

edited

Loading

After discussions with @choumx, I've loosened up this implementation to allow for usage of the amp-state: protocol even after init.

Note that this does create a potentially confusing situation where any async-created amp-state would not be present for the amp-list initialization. For example:

<amp-state id="xhr-state", src="http:/....."/>
<amp-list src="amp-state:xhr-state">
  ...
</amp-list>

samouri force-pushed the point-amp-state branch from 4ca6138 to 9d3ecdb Compare

January 11, 2020 00:24

samouri changed the title ~~✨ Allow amp-list to initialize off amp-state.~~ ✨ [amp-list] Introduce "amp-state:" as a usable protocol for the src attribute.

samouri marked this pull request as ready for review

January 11, 2020 00:55

amp-owners-bot bot requested a review from dreamofabear

January 11, 2020 00:55

dreamofabear commented Jan 13, 2020

I was curious if hyphens were allowed in URI schemes and looks like they are:

A non-empty scheme component followed by a colon (:), consisting of a sequence of characters beginning with a letter and followed by any combination of letters, digits, plus (+), period (.), or hyphen (-).

Member Author

samouri commented Jan 13, 2020

There was a a failing percy test, but after a rerun it passed.

samouri force-pushed the point-amp-state branch from f0af028 to 4ac091a Compare

January 13, 2020 17:54

Member Author

samouri commented Jan 13, 2020 •

edited

Loading

Note that this does create a potentially confusing situation where any async-created amp-state would not be present for the amp-list initialization. For example:

I'm going to chase this case in a followup pr. It probably makes sense to have undefined|null somehow turn into an in-progress/fetching state for amp-list (e.g. Promise<JsonObject> being returned by amp-bind apis).

dreamofabear commented Jan 13, 2020

@ampproject/wg-ui-and-a11y Any opinions on adding this feature in amp-list-0.1 vs. a new version?

Member Author

samouri commented Jan 13, 2020

@ampproject/wg-ui-and-a11y Any opinions on adding this feature in amp-list-0.1 vs. a new version?

Why would this be an issue? I don't think anything here breaks backwards compatibility.

dreamofabear commented Jan 13, 2020

This could be a good chance to clean up the API/code and also offer a carrot for adoption.

dreamofabear commented Jan 13, 2020

To be clear, I think this PR is fine to merge as is-is. We can discuss more before actual launch.

samouri self-assigned this

samouri added the WG: runtime label

dreamofabear requested a review from caroqliu

January 14, 2020 16:01

dreamofabear reviewed

View reviewed changes

dreamofabear left a comment

Looking good. Can we add an example test case to examples/bind/list.amp.html?

Also in case you were wondering, that file should probably live in test/manual/ (no action required).

extensions/amp-list/0.1/amp-list.js Outdated Show resolved Hide resolved

extensions/amp-list/0.1/amp-list.js Outdated Show resolved Hide resolved

extensions/amp-list/0.1/amp-list.js Outdated

+                      const errorMsg =
+                        'You must include amp-bind in order to use amp-state as an initial source for amp-list.';
+                      user().error(TAG, errorMsg);
+                      return Promise.resolve();

dreamofabear Jan 14, 2020

fetchList_() catches network fetch errors to perform fallback behaviors e.g. showing a [fallback] element if it exists. We should probably do the same here, which makes userAssert above a reasonable option.

Member Author

samouri Jan 14, 2020

Hmm. This actually makes it more similar to fetchList_ than to the flow of rendering computed bind expressions (which don't have a fallback). Considering the async nature of the state retrieval, I'll merge it with fetchList_

extensions/amp-list/0.1/amp-list.js Outdated Show resolved Hide resolved

extensions/amp-list/0.1/amp-list.js Outdated Show resolved Hide resolved

Member Author

samouri commented Jan 14, 2020

Thank you for the thoughtful feedback @choumx! I'll try to address all your comments

amp-bundle-size bot requested a review from danielrozenberg

January 14, 2020 19:11

samouri force-pushed the point-amp-state branch from 709c945 to be5a1fc Compare

January 14, 2020 20:37

caroqliu reviewed

View reviewed changes

extensions/amp-list/0.1/amp-list.js Outdated Show resolved Hide resolved

extensions/amp-list/0.1/amp-list.js Outdated Show resolved Hide resolved

caroqliu approved these changes

View reviewed changes

examples/bind/basic.amp.html Outdated Show resolved Hide resolved

danielrozenberg removed their request for review

January 15, 2020 19:28

samouri force-pushed the point-amp-state branch from a85bfc2 to 21d49e3 Compare

January 16, 2020 00:16

jridgewell reviewed

View reviewed changes

extensions/amp-list/0.1/amp-list.js Outdated Show resolved Hide resolved

extensions/amp-list/0.1/amp-list.js Outdated Show resolved Hide resolved

Member Author

samouri commented Jan 22, 2020

In a followup PR I'll still need to ensure that amp-state URIs cannot be used in email (assuming the allows regex rules)

samouri added 22 commits

January 27, 2020 17:41


          add test placeholders

0ab86d9


          finish adding in tests

0cbabf0


          remove trims

27fb62e


          privatify

45bc722


          remove support for post-init amp-state protocol

8dcd690


          add new unit test for attempts to set post-init src to amp-state

0bac916


          loosen it all up

4d59a23


          fetch always means fetch

92d906e


          make a clearer distinction between amp-stae: renders and bind expr re…

bc53467

…nders


          Address choumx's feedback.

19c189f


          user().assert --> userAssert

aeb9880


          add in example

a2b07e0


          Revert some changes now that amp-state protocol is more similar to a …

f5a789c

…fetch than to synchronous local data


          Caro updates

7003ff5


          WHOOPS

4952f61


          Promise<*> for getAmpStateJson

8a26381


          Emit warning for the // can never happen // case

80a2558


          resolve jridgewell comments

db5d9bd


          restrict amp-state: to just initialization.

f4f2eda


          fix tests


          simplify fetch expectations and make more consistent with xhr

daeeacb


          fix basic example

09fdd96

samouri force-pushed the point-amp-state branch from cfeeca2 to 09fdd96 Compare

January 27, 2020 22:41

jridgewell approved these changes

View reviewed changes

Contributor

jridgewell left a comment

Thanks for updating!

samouri merged commit d5b7775 into ampproject:master

samouri deleted the point-amp-state branch

January 28, 2020 16:20

westonruter added a commit to westonruter/amphtml that referenced this pull request


          Merge branch 'master' of github.com:ampproject/amphtml into add/amp-i…

2b6e016

…frame-pymjs-support

* 'master' of github.com:ampproject/amphtml: (436 commits)
  🐛Add computed styles to fake win (ampproject#26514)
  📦 Update dependency eslint-config-prettier to v6.10.0 (ampproject#26518)
  🐛 Rewrite relative urls in the bookend (ampproject#26490)
  ✨ [amp-list] Introduce "amp-state:" as a usable protocol for the src attribute. (ampproject#26284)
  Add terminal newline to files.txt (ampproject#26513)
  📦 Update dependency chromedriver to v79.0.2 (ampproject#26515)
  ♻️ Change the opt-in cookie values to reflect upcoming CDN changes (ampproject#26489)
  ♻️<amp-next-page> v2 minor renaming and fixes (ampproject#26468)
  fix link (ampproject#26508)
  amp-list: fix broken test re. missing layout (ampproject#26509)
  performance: emit timeOrigin w/ polyfill (ampproject#26485)
  Position n+1 story desktop page before positioning attributes are set. (ampproject#26488)
  ✨amp-nested-menu: allow svg into (ampproject#26502)
  Log user warning when missing URL arg for shadow doc (ampproject#26290)
  📦 Update dependency puppeteer to v2.1.0 (ampproject#26501)
  Ensure that fluid slots hidden by media queries do not issue ad requests. (ampproject#26352)
  📦 Update dependency mocha to v7.0.1 (ampproject#26495)
  📦 Update dependency fetch-mock to v8.3.2 (ampproject#26491)
  Revert 'Move mutator implementations out to a standalone service' (ampproject#26479)
  Fix syntax error (ampproject#26481)
  ...

samouri mentioned this pull request

wg-runtime Status Update 2020-02-03 ampproject/wg-runtime#28

Closed

ampprojectbot added PR use: In Stable PR use: In LTS labels

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes PR use: In LTS PR use: In Stable WG: runtime