download-runtime: Add tool to download AMP runtime#663
download-runtime: Add tool to download AMP runtime#663sebastianbenz merged 11 commits intoampproject:masterfrom
Conversation
There was a problem hiding this comment.
@sebastianbenz I ran out of steam when it came to writing tests. Since I'm unsure how much time I'll have to put towards this tool during the coming week, I thought I'd post what is otherwise a finished tool in case you'd like to review implementation.
EDIT: I found some time to add tests. I'm happy to accept reviews when you (and others!) have time.
packages/cli/package.json
Outdated
| "dependencies": { | ||
| "@ampproject/toolbox-cache-list": "^2.0.0", | ||
| "@ampproject/toolbox-cache-url": "^2.0.0", | ||
| "@ampproject/toolbox-download-framework": "file:../../packages/download-framework", |
There was a problem hiding this comment.
TODO: Revise this to "^2.00" once testing is complete.
There was a problem hiding this comment.
You should be able to set this to 2.0.0 as lerna takes care of managing local dependencies.
| expect(ret.dest).toBe(options.dest); | ||
| expect(ret.rtv).toBe(defaultRtv); | ||
| done(); | ||
| }); |
There was a problem hiding this comment.
Should also readdir and expect all framework files made it to the destination dir.
| @@ -0,0 +1,160 @@ | |||
| /** | |||
| * Copyright 2017 The AMP HTML Authors. All Rights Reserved. | |||
sebastianbenz
left a comment
There was a problem hiding this comment.
Looks good! Thanks a lot! Left a few suggestions, mostly around naming.
packages/cli/lib/cli.js
Outdated
| switch (command) { | ||
| case 'curls': | ||
| return require('./cmds/curls')(args, this.logger_); | ||
| case 'download-framework': |
There was a problem hiding this comment.
suggestion: rename to simply download
packages/cli/package.json
Outdated
| "dependencies": { | ||
| "@ampproject/toolbox-cache-list": "^2.0.0", | ||
| "@ampproject/toolbox-cache-url": "^2.0.0", | ||
| "@ampproject/toolbox-download-framework": "file:../../packages/download-framework", |
There was a problem hiding this comment.
You should be able to set this to 2.0.0 as lerna takes care of managing local dependencies.
README.md
Outdated
| - **[amp-cache-list](./packages/cache-list):** a javascript library for listing the known AMP Caches. | ||
| - **[amp-cli](./packages/cli):** a command line version of AMP Toolbox | ||
| - **[amp-cors](./packages/cors):** a connect/express middleware to automatically add [AMP Cors headers](https://www.ampproject.org/docs/fundamentals/amp-cors-requests). | ||
| - **[amp-download-framework](./packages/download-framework):** a javascript library for downloading the AMP framework. |
There was a problem hiding this comment.
Discussion: I'd prefer to rename this to runtime-download. Personally, I find framework a bit confusing in this context.
| const downloadFrameworkProvider = require('@ampproject/toolbox-download-framework'); | ||
|
|
||
| async function downloadFramework(args, _) { | ||
| const {ampUrlPrefix, clear, dest, rtv} = args; |
There was a problem hiding this comment.
We need to check if dest has been provided. Would it make sense to use rtv/$RTV_VERSION/ as the default value for dest? This way it'd be easy to replicate the cache fs structure.
| const filesTxtUrl = frameworkBaseUrl + FRAMEWORK_FILES_TXT; | ||
|
|
||
| ret.url = frameworkBaseUrl; | ||
| log.info('AMP framework base URL: ' + frameworkBaseUrl); |
There was a problem hiding this comment.
remove framework from log string
| throw new Error(`Expected ${FRAMEWORK_FILES_TXT} in file listing, but it was not found.`); | ||
| } | ||
| } catch (ex) { | ||
| ret.error = 'Unable to read AMP framework files listing\n' + ex.message; |
There was a problem hiding this comment.
remove framework from log string
| // Create all subdirectories in destination directory | ||
| this.createSubdirectories_(files, dest); | ||
|
|
||
| log.info('Downloading AMP framework...'); |
There was a problem hiding this comment.
s/framework/runtime/
| await Promise.all(fetchAndSavePromises) | ||
| .then(() => { | ||
| ret.status = true; | ||
| log.info('AMP framework download complete: ' + dest); |
There was a problem hiding this comment.
remove framework from log string
| log.info('AMP framework download complete: ' + dest); | ||
| }) | ||
| .catch((error) => { | ||
| ret.error = 'Failed to download AMP framework\n' + error.message; |
There was a problem hiding this comment.
s/framework/runtime
* framework --> runtime * Copyright year * Random tmp dir for testing * Reformat for prettier * cli/package.json dependency * CLI command 'download' * Download to rtv-specific paths
|
Tool tested on Windows successfully: and unit tests (for this tool) pass as well: |
|
Great - thanks! Can you take a look at the travis build? Looks like Node 10 and 11 fails. |
|
Oh, I see. Recursive rmdir not available until node 12. Will fix. |
| "cross-fetch": "3.0.4", | ||
| "fetch-mock": "9.2.1" | ||
| "fetch-mock": "9.2.1", | ||
| "fs-extra": "^9.0.0" |
There was a problem hiding this comment.
please pin the version
| "@ampproject/toolbox-runtime-version": "^2.0.0", | ||
| "cross-fetch": "3.0.4", | ||
| "fetch-mock": "9.2.1" | ||
| "fetch-mock": "9.2.1", |
There was a problem hiding this comment.
Please remove. This should already be included as a dependency (in the root package).
| '--dest (required).......... Destination directory', | ||
| '--clear (optional)......... Clear destination directory before download (default: true)', | ||
| '--rtv (optional)........... Runtime version to download (default: latest available)', | ||
| '--ampUrlPrefix (optional).. AMP host (default: https://cdn.ampproject.org)', |
There was a problem hiding this comment.
should we rename this to host?
| @@ -0,0 +1,36 @@ | |||
| { | |||
| "name": "@ampproject/toolbox-download-runtime", | |||
| "version": "2.0.0", | |||
There was a problem hiding this comment.
this needs to be 2.0.1 for lerna to pick this up as a local dependency.
sebastianbenz
left a comment
There was a problem hiding this comment.
Left a few more comments. Only nits.
One thing: what I meant with making dest default to rtv/$RTV was that we use this as the default value, but allow full customization (without rtv prefix). Thinking about this I like your approach more, but I'm not sure whether this would work for everyone. Wdyt?
- Support `amp download` without any options to CLI tool. Defaults to `amp-runtime` folder within current working directory as destination. - Remove redundant package specification in CLI package.json - Set download-runtime version to 2.0.1 - Update CLI readme - Fix 'supports disabling destination dir clearing' test - Rename --ampUrlPrefix flag to --host for CLI tool
I like the idea of downloading to I took your suggestion to make Other minor updates are included in latest commit as well. Please let me know if I've left anything unaddressed and thank you much for all the reviews! |
sebastianbenz
left a comment
There was a problem hiding this comment.
Love it! Thanks a lot!
|
@sebastianbenz a couple docs need some minor tweaks since you removed |
Add tool to download a complete, compiled AMP runtime (and components). CLI and module included.
Supported CLI options:
--dest(optionalstring): Specify the destination directory where the AMP framework should be saved. Defaults toamp-runtimefolder in current working directory.--clear(optionalboolean): Remove all contents from the destination directory before saving the AMP framework. Defaults totrue.--rtv(optionalstring): Specify the runtime version to download. Defaults to the latest production version available.--host(optionalstring): Specify the URL where the AMP framework is hosted. Defaults tohttps://cdn.ampproject.org.Fixes #642