It would be useful to clarify what the "old version" refers to.

+1, here and elsewhere in the PR

I don't think we do this type of line alignment in our codebase.

nit: +1 space

If we put this inline as a component, we need to figure out how to communicate:

(1) Don't communicate; allow reordering: I would assume this to be the default; wherever publishers put the share-providers component, it shows up. This is problematic because we generally want to ensure that it comes first, that there is at most one, and that if unspecified, we add it as first anyway.
(2) Don't communicate; disallow reordering: We take the first share-providers instance we find and make it the first component. If they specify more than one, we silently drop the rest (or somehow merge them).
(3) Console error: If they specify a share-providers component as anything other than the first component, we throw a console error and disregard that component entirely. This provides flexibility, guarantees there is only one, and guarantees that it comes first, but is potentially confusing to publishers (e.g. if they don't look at the console)
(4) Separate entity: We can move the share-providers out of the components object (as it was before), and we insert it as the first thing at runtime. This guarantees there is only one and that it is first, but is relatively unflexible.

Hmm I'm leaning more towards moving it out of the components object, as it was before. It seems that with any approach that we take, we would enforce the position of the share-providers at some level.

Having it outside of the component object seems to align with these semantics.

nit: one of these refers to a key and one refers to a value, but the names don't make that clear

Maybe console.warn('Version 1 of the amp-story bookend is deprecated. Use version 2: [link]')

As a side note, we should publish docs somewhere so that [link] can link to something 😄 That part can come in a separate PR.

Ok, will do this in a follow-up PR after this 👍

+1, here and elsewhere in the PR

What's the TODO?

Same, re: "new" API

I think we should explicitly typedef the different components. Like:

/**
 * @typedef {{
 *   type: string,
 *   title: string,
 *   url: string,
 *   image: (string|undefined)
 * }}
 */
let BookendArticleComponentDef;

/**
 * @typedef {{
 *   type: string,
 *   text: !Array<string>
 * }}
 */
let BookendTextComponentDef;

Then, union the different components for this:

/** @typedef {!BookendArticleComponentDef|!BookendTextComponentDef|...} */
let BookendComponentDef;

nit: s/undefinded/undefined/

@newmuis not sure what you think of this interface. It looks cleaner to me to just write the providers name, and in the case of facebook make it an object with the app_id as the value. Downside is that maybe it's not as verbose as writing {"provider": "facebook", "app_id": "1235"}. WDYT?

There are other options that one may or may not need to set, apart from the app id;. This format loses the ability to specify more than one parameter per provider.

Got it. Will make the change.

Should we keep this consistent with the amp-story version number? The current version would be 0.1, and this one 1.0.
Might be confusing for external developers that amp-story 0.1 uses the bookend v1, and amp-story 1.0 uses the bookend v2.

Yeah I was thinking about it. Lets do it!

The mutate phase of a vsync is meant for write operations.
Please move the getInnerContainer into the measure phase to make sure read/write operations are properly batched by the vsync tasks implementation to avoid layout thrashing.

this.vsync_.run({
  measure: state => {
    state.innerContainer = this.getInnerContainer_();
  },
  mutate: state => {
    state.innerContainer.appendChild(...);
  },
}, {})

drive-by: we are trying to move away from using vsync. Same idea but preferred method is baseElement.measureMutateElement()

Nit: @param {!Array<!Object|String>}

It's actually returning an object, the return might look like

@return {!Object<string, boolean|!Object<string, string>>}

Woops, I think I got the param and return mixed up. Thanks for looking.

Nit: this name hints at a list of objects, when it's actually an object (or a Map?)
What do you think if naming this providersMap or anything else more accurate?

Nit: Please use a forEach here, as you're not using the returned array generated by the map function

Nit: please be more specific on which part needs to be removed

Since you'll have to do this for every bookend component, what do you think of a validate method in the AbstractBookendComponent?

Should you validate that this string exists too?

We want to migrate away from using simple templates and instead use the static HTML template helper where possible: #14659

nit: version

nit: use html instead of htmlFor(doc)

Simpler:

return components.reduce((acc, component) => {
  const klass = componentClassFor(component);
  if (!klass || !klass.isValid(component)) {
    return;
  }
  acc.push(klass.build(component));
  return acc;
}, []);

We have a loosely-enforced rule of not allocating complex objects on module import due to parsing lifecycle. This can be mitigated by replacing componentsMap with a simple factory function containing a switch clause.

Using said function in examples below.

This should be replaced with an interface.

You could save a few bytes from the binary by returning the assertions:

return user().assert(false, 'Articles must contain `title` and `url` fields, skipping invalid.');

Awesome, thanks for using static-template!

Small note, the binaries resulting from usages of htmlFor are a bit bloated. If you want, you can add a comment including the tracking bugs #14657, #14658

It's better to return from the special case and set the "meat" of the method below:

if (response[BOOKEND_VERSION_KEY] === BOOKEND_VERSION_1) {
  // ...
  return;
}
dev().warn(TAG, `Version ${BOOKEND_VERSION_0} of the amp-story`);
// ...

But then the rest of the function won't be ran after the break, right? We still want to run whatever's after the if-else.

if (response[BOOKEND_VERSION_KEY] === BOOKEND_VERSION_1) {
  // ...  
  return;
}

// This won't run anymore:

if (applyConfig) {
    this.setConfig_(this.config_);
}

return this.config_;

Inline mutate + assert-clause. You also don't need to put a DOM query in a measure context. You can also build outside the mutate, to unload the mutate batch.

dev().assertElement(this.bookendEl_, 'Error rendering amp-story-bookend.');
const fragment = BookendComponent.buildTemplates(components, this.win_.document));
const container =  this.getInnerContainer_();
this.resources_.mutate(() => container.appendChild(fragment));

@jridgewell @calebcordry recommended me to stay away from vsync, and I don't think resources has a mutate property. Maybe with the mutateElement one? 🤔

You can save a few bytes by returning the assertion directly:

return user().assert(false, 'Articles must contain...');

Not entirely sure what the purpose of this is. Could it be made generic? Could the different cases be explained by virtue of naming vars or comments?

Also, simpler:

if (!isObject(currentProvider)) {
  providersMap[currentProvider] = true
  return;
}
if (currentProvider['provider'] == 'facebook') {
  providersMap['facebook'] = ({'app_id': currentProvider['app-id']});
  return;
}
providersMap[currentProvider['provider']] = true;

Use isArray from src/types