Importing component scripts (FR #1457) #1513

garanj · 2018-09-11T13:50:08Z

A start point for the FR from @tomayac #1457.

In summary, this PR adds the feature to automatically insert <script> elements for AMP components to the <head> element, when the user adds code requiring that component.

Frontend

The frontend adds a module auto-importer which listens for events from the AMP Validator. Where the validation result is a failure, the errors are filtered to determine whether any correspond to missing imports.

The resulting missing imports list is cross-referenced with the parsed state of the <head> element (as the former can sometimes report missing elements that are actually present), to arrive at a list of elements to insert.

The logic for insertion is as follows:

If the AMP engine <script> element already exists, insert any missing <script> component elements directly beneath it.
If the AMP engine does not exist, then insert both the AMP engine and required component elements as the last elements in the <head> element.

Currently more complex scenarios seeking to group together existing elements, sort them etc are not dealt with, but the output of parsing the <head> element does contain what would be required for this.

Backend

The frontend, on initialization, obtains a mapping of AMP component to version number from the backend, using the endpoint /playground/amp-component-versions

This mapping is formed from the GitHub AMP extensions directory structure (thank you @sebastianbenz).

The mapping returned by the endpoint is stored in Memcache, backed by Datastore. Requests that cause a stale list to be served, trigger an update which is achieved through a named TaskQueue entry.

Demo

An demo can be seen in this video

garanj · 2018-09-11T14:26:49Z

lint:backend now works: In short, wrong gofmt version...

sebastianbenz

Looks great already. Left a few comments, but mostly nits.

I've noticed one problem: if you paste the following document into the playground it'll also import the amp-lightbox-gallery component.


<!doctype html>
<html ⚡>
<head>
  <meta charset="utf-8">
  <title>My AMP Page</title>
  <link rel="canonical" href="self.html" />
  <meta name="viewport" content="width=device-width,minimum-scale=1">
  <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
  <script async src="https://cdn.ampproject.org/v0.js"></script>
  
  <style amp-custom>
    h1 {
      margin: 1rem;
    }
  </style>
</head>
<body>
  <h1>Hello AMPHTML World!</h1>
  <amp-carousel layout=fill>
  </amp-carousel>
  <amp-iframe src="https://example.com" layout=fill></amp-iframe>
</body>
</html>

playground/serve.go

sebastianbenz · 2018-09-17T11:02:52Z

playground/serve.go

 func InitPlayground() {
 	http.HandleFunc("/playground/fetch", handler)
+	http.HandleFunc("/playground/amp-component-versions", components)


nit: extract the playground path prefix /playground into a constant

sebastianbenz · 2018-09-17T11:07:06Z

playground/serve.go

+		return false, err
+	}
+	addComponentsToStores(ctx, list)
+	return true, nil


Remove the bool return value as it's not needed.

sebastianbenz · 2018-09-17T11:07:50Z

playground/serve.go

+	return true, nil
+}
+
+func getComponentsFromDataStore(ctx context.Context) (*AmpComponentsList, error) {


nit: s/get/fetch

get implies returning a value that's already there, whereas fetch indicates that it's potentially a long running operation.

sebastianbenz · 2018-09-17T11:08:16Z

playground/serve.go

+
+func getComponentsFromDataStore(ctx context.Context) (*AmpComponentsList, error) {
+	log.Infof(ctx, "Retrieving components from datastore")
+	var list AmpComponentsList


nit: s/list/components

sebastianbenz · 2018-09-17T11:30:41Z

playground/src/auto-importer/auto-importer.js

+  }
+
+  update(validationResult) {
+    if (validationResult.status === 'FAIL') {


nit: use an early exit to avoid nesting

if (validationResult.status !== 'FAIL') { return }

sebastianbenz · 2018-09-17T11:33:49Z

playground/src/auto-importer/auto-importer.js

+        }
+      }
+    }
+    return {


suggestion: create the return obj at the beginning of the method

const missingElements = { components: {}, ampRuntime: false }

sebastianbenz · 2018-09-17T11:33:57Z

playground/src/auto-importer/auto-importer.js

+   * missingTags: The keys of this dictionary are those missing AMP components.
+   * missingBaseScriptTag: True if the AMP engine <script> tag is not present.
+   */
+  _getMissingElements(validationResult) {


suggestion: rename s/_get/_parse

sebastianbenz · 2018-09-17T11:41:28Z

playground/src/auto-importer/auto-importer.js

+   */
+  _insertMissingElements(missing, existing) {
+    const pos = existing.baseScriptTagEnd || existing.lastTag;
+    if (pos) {


again: early exit if(!pos) return to avoid nesting

sebastianbenz · 2018-09-17T11:53:51Z

playground/src/auto-importer/auto-importer.js

+import CodeMirror from 'codemirror';
+
+const MISSING_ENGINE_PARAM = 'amphtml engine v0.js script';
+const AMP_BASE_URL = '"https://cdn.ampproject.org/v0.js"';


How hard would it be to also support auto-adding the runtime for amp ads?

I think this would be pretty straightforward. I can give it a go?

Sure! But only if it doesn't take too much effort.

I've added this.

garanj · 2018-09-17T19:19:57Z

Thanks @sebastianbenz for the comments! I will work through them!

On the <amp-carousel> example, this of course comes straight back from the validator, as per on the command-line (example.html containing your HTML):

$ amphtml-validator example.html
example.html:19:2 The mandatory attribute 'lightbox' is missing in tag 'AMP-CAROUSEL [lightbox]'. (see https://www.ampproject.org/docs/reference/components/amp-carousel)
example.html:19:2 The tag 'AMP-CAROUSEL [lightbox]' requires including the 'amp-carousel' extension JavaScript. (see https://www.ampproject.org/docs/reference/components/amp-carousel)
example.html:19:2 The tag 'AMP-CAROUSEL [lightbox]' requires including the 'amp-lightbox-gallery' extension JavaScript. (see https://www.ampproject.org/docs/reference/components/amp-carousel)
example.html:21:2 The tag 'amp-iframe' requires including the 'amp-iframe' extension JavaScript. (see https://www.ampproject.org/docs/reference/components/amp-iframe)

From what I can see in the validator spec for amp-carousel, it looks like the wrong TagSpec is being identified for the amp-carousel: the lightbox mode one compared with the plain one.

I can open a bug against the validator for this one? I think that's probably the appropriate place to try to fix this? What do you reckon? (I ask as I am very new to this....!)

sebastianbenz · 2018-09-17T19:25:51Z

Hah! You're right! Please file a bug against the validator.

garanj · 2018-09-17T19:51:22Z

Added: ampproject/amphtml#18091 for the amp-carousel issue.

garanj · 2018-09-18T13:44:54Z

Hi Sebastian, I think I've made a stab at making the changes. Thank you for the review

sebastianbenz

Great thanks! Few nits.

I've also deployed it, but there seems to be a problem Warning: Unknown AMP component : amp-carousel: https://amp-by-example-staging.appspot.com/playground/

sebastianbenz · 2018-09-18T14:12:41Z

playground/src/components-provider/components-provider.js

+  }
+
+  getComponents() {
+    return this.componentsMap;


Can you make this return a promise?

Yes, sorry, this was rubbish.

sebastianbenz · 2018-09-18T14:38:14Z

playground/src/components-provider/components-provider.js

+    this.win.requestIdleCallback(() => this._fetchComponents());
+  }
+
+  getComponents() {


suggestion: rename to simply get as the class has only one functionality.

sebastianbenz · 2018-09-18T14:38:34Z

playground/serve.go

+const (
+	COMPONENTS_URL                 = "https://api.github.com/repos/ampproject/amphtml/git/trees/master?recursive=1"
+	COMPONENTS_MEMCACHE_KEY        = "amp-components-list"
+	COMPONENTS_UPDATE_FREQ_SECONDS = 86400


nit: add a comment

COMPONENTS_UPDATE_FREQ_SECONDS = 86400 // one day

garanj · 2018-09-18T15:18:27Z

Apologies for that Sebastian, I believe it's fixed now. Thanks for the suggestion, my original approach was bad.

sebastianbenz · 2018-09-18T15:29:38Z

playground/src/auto-importer/auto-importer.js

@@ -40,28 +40,28 @@ class AutoImporter {

  constructor(componentsProvider, editor) {
    this.componentsProvider = componentsProvider;
-    this.componentsMap = {};
+    this.componentsMap = componentsProvider.get();


I'd simply assign the components provider as a field. Otherwise a better approach would be to pass the componentsPromise via the constructor.

sebastianbenz · 2018-09-18T16:02:18Z

playground/src/auto-importer/auto-importer.js

-    }
+    this.componentsMap.then((components) => {
+      this.components = this.components || components;
+      if (validationResult.status !== 'FAIL') {


Move this to the beginning of the method

sebastianbenz · 2018-09-18T16:03:33Z

playground/src/auto-importer/auto-importer.js

-      this._insertMissingElements(missing, existing);
-    }
+    this.componentsMap.then((components) => {
+      this.components = this.components || components;


I'd prefer if you'd make the state handling more explicit by passing the components variable to _insertMissingElement instead of assigning it to a field.

sebastianbenz · 2018-09-18T16:05:08Z

playground/src/components-provider/components-provider.js

+            .then((r) => r.json())
+            .then((data) => {
+              resolve(data);
+            });


What happens if the fetch fails?

How about a number of backoff retries, then ultimately if there is no success after n tries, assign the the components map to just be empty? What do you think?

As it's not a critical feature I think it's fine to simply fail and fallback to an empty map at first. We can add a retry later.

sebastianbenz

Thanks for the fixes. It still doesn't work when deployed (see my other comment).

I've also noticed another thing we need to change: currently the auto import is triggered as soon as I type: amp-access. However, I might actually want to include amp-access-laterpay. Is there a way to avoid this?

sebastianbenz · 2018-09-19T08:51:34Z

playground/serve.go

+	log.Infof(ctx, "Retrieving components from memcache")
+	var components AmpComponentsList
+	_, err := memcache.Gob.Get(ctx, COMPONENTS_MEMCACHE_KEY, &components)
+	if err == memcache.ErrCacheMiss {


When I deploy the version, the result is always {}. It's related to this part as it works when I replace this line with if err != nil {. BTW this is only a problem when I deploy it to app engine, it works locally.

In general can you make sure to handle the errors correctly:

can you log all errors: log.Errorf(ctx, "..... %v", err)

can you make sure to return a corresponding status code if we could not retrieve the list of components.

garanj · 2018-09-20T13:39:56Z

Regarding adding components too early, one option might be to inspect the token just before the cursor when a validation error takes place using cm.getTokenAt(). If this is of type tag, then stop the auto-import, as the cursor is within an element tag.

This would mean auto import wouldn't happen til the tag was closed or a space entered to then add attributes.

e.g:

const currentTag = this.editor.getTokenAt(this.editor.getCursor());
if (currentTag.type === 'tag') {
  return;
}

Thoughts?

sebastianbenz · 2018-09-20T14:52:22Z

This sounds like a perfect solution!

garanj · 2018-09-21T13:13:51Z

I've added:

Handling of errors from GitHub better, and use of authenticated requests to the API
Better logging of errors, including passing back any GitHub HTTP error code through to the client
Delaying auto-import until outside of a tag entry

sebastianbenz · 2018-09-21T13:26:21Z

Great! Taking a look. Can you also rebase against master (to fix the merge conflict).

sebastianbenz

Great! Only a few nits, but then we're good to merge!

sebastianbenz · 2018-09-21T14:01:41Z

playground/serve.go

+			return dsComponents, nil
+		}
+	} else if err != nil {
+		return nil, &ComponentsReqError{"Memcache error", http.StatusInternalServerError}


We should log this error. However, we should also fallback to loading the components in this case. Suggestion:

if err == nil { return &components, nil } if err == memcache.ErrCacheMiss { log.Infof(ctx, "Components not in memcache, retrieving from datastore") } else { log.Errorf(ctx, "Error when retrieving components from memcache: %v, %v", err.Code, err.Message) } dsComponents, err := fetchComponentsFromDataStore(ctx) ...

Great! Done

sebastianbenz · 2018-09-21T14:02:01Z

playground/serve.go

+	key := datastore.NewKey(ctx, "AmpComponentsList", "ComponentsListKey", 0, nil)
+	err := datastore.Get(ctx, key, &components)
+	if err != nil {
+		log.Warningf(ctx, "Error retrieving components from datastore")


can you also log the error

garanj · 2018-09-21T22:09:05Z

Should I add some documentation anywhere about use of GitHub API key, and how to set it up in DataStore? Does it fit in the README.md, or is it something that anyone deploying would know already..?

sebastianbenz

Thanks a lot! Would be great if you could add the datastore info into a readme inside the playground dir.

garanj · 2018-09-25T14:15:52Z

I've added a readme about adding the token to Datastore.

sebastianbenz · 2018-09-25T14:19:47Z

Fantastic Thanks!

tomayac · 2018-09-25T14:23:11Z

Brilliant, @garanj! Thank you so much for making this happen! 👏

garanj added 4 commits September 9, 2018 22:28

Added AMP component auto-import.

c692a0d

Minor formatting fixes.

0ed2a98

Reformatted Go-style if... statements

e49bd05

Format Go code

67f90bb

garanj added 3 commits September 11, 2018 21:42

Go formatting

1d27733

Merge remote-tracking branch 'upstream/master'

3ee4001

Go formatting

27f7620

sebastianbenz self-requested a review September 17, 2018 08:40

sebastianbenz suggested changes Sep 17, 2018

View reviewed changes

Changes from review

b88c827

sebastianbenz suggested changes Sep 18, 2018

View reviewed changes

Changes to ComponentsProvider

c13c672

sebastianbenz reviewed Sep 18, 2018

View reviewed changes

Requested changes to ComponentsProvider

f219e79

sebastianbenz suggested changes Sep 19, 2018

View reviewed changes

garanj added 2 commits September 21, 2018 14:03

Fixed handling of GitHub errors

f578702

Comments explaining use of API key

0b6cf85

sebastianbenz reviewed Sep 21, 2018

View reviewed changes

Added error message from Datastore

c480cb1

sebastianbenz approved these changes Sep 25, 2018

View reviewed changes

Added README for Datastore configuration

057ecf4

sebastianbenz merged commit 382029c into ampproject:master Sep 25, 2018

Importing component scripts (FR #1457) #1513

Importing component scripts (FR #1457) #1513

Conversation

garanj commented Sep 11, 2018 • edited Loading

Frontend

Backend

Demo

garanj commented Sep 11, 2018 • edited Loading

sebastianbenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garanj commented Sep 17, 2018

sebastianbenz commented Sep 17, 2018

garanj commented Sep 17, 2018

garanj commented Sep 18, 2018

sebastianbenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garanj commented Sep 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastianbenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garanj commented Sep 20, 2018

sebastianbenz commented Sep 20, 2018

garanj commented Sep 21, 2018

sebastianbenz commented Sep 21, 2018 • edited Loading

sebastianbenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garanj commented Sep 21, 2018

sebastianbenz left a comment

Choose a reason for hiding this comment

garanj commented Sep 25, 2018

sebastianbenz commented Sep 25, 2018

tomayac commented Sep 25, 2018

garanj commented Sep 11, 2018 •

edited

Loading

garanj commented Sep 11, 2018 •

edited

Loading

sebastianbenz commented Sep 21, 2018 •

edited

Loading