Add AutoExtensionImporter #569
Conversation
sebastianbenz
force-pushed
the
auto-importer
branch
from
38a4fd5
to
85a29d8
Compare
|
@fstanis great review! thanks a lot! PTAL |
|
@fstanis , yes, this looks so thorough that I don't feel obliged to take a look myself! As long as you've looked a bit at https://github.com/garanj/amphtml-autoscript to make sure you don't repeat any of our mistakes :) |
|
/cc @garanj |
| async initExtensionSpec_(validatorRules) { | ||
| this.extensionSpec = validatorRules.fetch().then((rules) => { | ||
| // Map extension names to more info | ||
| const extensionsMap = new Map(rules.extensions.map((ext) => [ext.name, { |
There was a problem hiding this comment.
Thinking about this... you may want to take htmlFormat into account. For example, AMP for Email doesn't support amp-carousel 0.2.
More importantly, this assumes there's exactly one extension for a given ext.name which is incorrect because of the above. I'm not sure what the current behavior is, but it will give you an arbitrary amp-carousel that may be the email one (and thus 0.1) or the regular one (and thus 0.1 and 0.2).
There was a problem hiding this comment.
Yup. That's already on my todo list.
- Support multiple formats - Add validador-rules dependency to optimizer
There was a problem hiding this comment.
I've added format support. The default is AMP for websites, but other formats can be set via the config. I thought about auto detecting the format, but I don't think it's worth the effort for now.
@fstanis PTAL
|
Forgot to mention earlier, the test runner automatically picks up tests (every folder is a separate test with input and expected output). |
| const extensionsMap = new Map(); | ||
| for (const ext of rules.extensions) { | ||
| if (ext.htmlFormat.includes(DEFAULT_FORMAT)) { | ||
| extensionsMap.set(ext.name, { |
There was a problem hiding this comment.
While the format check almost certainly ensures uniqueness... I'd urge adding if (extensionsMap.has(ext.name)) { throw ... } just in case.
There was a problem hiding this comment.
This was actually a bug: DEFAULT_FORMAT needs to be this.format. Throwing an exception here feels like overkill, this would mean if the validation format has a bug it'll mean pages will suddenly start to break which we should avoid if possible.
|
Thanks everyone for the feedback! |
No description provided.