From 049447aa0a04a313e3f87a5709322763663c1f21 Mon Sep 17 00:00:00 2001 From: erwin mombay Date: Tue, 5 Apr 2022 12:13:16 -0700 Subject: [PATCH 1/6] move file url calculation to extension-script.js (#38008) --- build-system/server/app-utils.js | 7 +-- build-system/server/app.js | 13 +++++ extensions/amp-story/1.0/amp-story.js | 10 ++-- .../amp-story/1.0/test/test-amp-story.js | 2 +- src/service/extension-script.js | 19 +++++++ test/unit/test-extension-script.js | 49 +++++++++++++++++++ 6 files changed, 90 insertions(+), 10 deletions(-) diff --git a/build-system/server/app-utils.js b/build-system/server/app-utils.js index ac011eae4d6a..65582bb541fa 100644 --- a/build-system/server/app-utils.js +++ b/build-system/server/app-utils.js @@ -72,7 +72,7 @@ const isRtvMode = (serveMode) => { */ const getCdnUrlRegExp = (pathPattern = '[^\'">]+') => new RegExp( - `(https://cdn\\.ampproject\\.org)/${pathPattern}(\\.m?js|\\.css)`, + `(https://cdn\\.ampproject\\.org)/${pathPattern}(\\.json|\\.m?js|\\.css)`, 'g' ); @@ -116,8 +116,9 @@ function replaceCdnJsUrls(mode, html, hostName, useMaxNames) { if (isRtv) { return CDNURLToRTVURL(url, mode, pathnames, extension).href; } - // CSS files are never output as .max.css - const useMaxNamesForFile = useMaxNames && extension !== '.css'; + // Only JS files have "max" files. Even mjs files don't have an equivalent + // max file during "amp build". + const useMaxNamesForFile = useMaxNames && extension === '.js'; const href = getHrefWithoutHost( replaceCDNURLPath(url, pathnames, extension, useMaxNamesForFile) ); diff --git a/build-system/server/app.js b/build-system/server/app.js index 671217091693..29d785dec7b9 100644 --- a/build-system/server/app.js +++ b/build-system/server/app.js @@ -1400,6 +1400,19 @@ app.get( } ); +/** + * Handle amp-story translation file requests with an rtv path. + * We need to make sure we only handle the amp-story requests since this + * can affect other tests with json requests. + */ +app.get('/dist/rtv/*/v0/amp-story*.json', async (req, _res, next) => { + const fileName = path.basename(req.path); + let filePath = 'https://cdn.ampproject.org/v0/' + fileName; + filePath = replaceUrls(SERVE_MODE, filePath); + req.url = filePath; + next(); +}); + if (argv.coverage === 'live') { app.get('/dist/amp.js', async (req, res) => { const ampJs = await fs.promises.readFile(`${pc.cwd()}${req.path}`); diff --git a/extensions/amp-story/1.0/amp-story.js b/extensions/amp-story/1.0/amp-story.js index 5dd1212e8d5c..f5003b0ab3c2 100644 --- a/extensions/amp-story/1.0/amp-story.js +++ b/extensions/amp-story/1.0/amp-story.js @@ -55,7 +55,7 @@ import {getHistoryState as getWindowHistoryState} from '#core/window/history'; import {isExperimentOn} from '#experiments'; import {Services} from '#service'; -import {calculateScriptBaseUrl} from '#service/extension-script'; +import {calculateExtensionFileUrl} from '#service/extension-script'; import {createPseudoLocale} from '#service/localization/strings'; import {getDetail} from '#utils/event-helper'; @@ -2668,14 +2668,12 @@ export class AmpStory extends AMP.BaseElement { fetchLocalizationStrings_(languageCode) { const localizationService = getLocalizationService(this.element); - const base = calculateScriptBaseUrl( + const localizationUrl = calculateExtensionFileUrl( + this.win, this.win.location, + `amp-story.${languageCode}.json`, getMode(this.win).localDev ); - const rtvPath = getMode(this.win).localDev - ? '' - : `rtv/${getMode(this.win).rtvVersion}/`; - const localizationUrl = `${base}/${rtvPath}v0/amp-story.${languageCode}.json`; // The cache fallbacks to english if language not found, locally it errors. Services.xhrFor(this.win) diff --git a/extensions/amp-story/1.0/test/test-amp-story.js b/extensions/amp-story/1.0/test/test-amp-story.js index 8cb6433ed042..60d905304ae6 100644 --- a/extensions/amp-story/1.0/test/test-amp-story.js +++ b/extensions/amp-story/1.0/test/test-amp-story.js @@ -2162,7 +2162,7 @@ describes.realWin( await createStoryWithPages(1, ['cover']); expect(fetchStub).to.have.been.calledOnceWithExactly( - '/dist/v0/amp-story.es-419.json', + '/dist/rtv/123/v0/amp-story.es-419.json', env.sandbox.match.any ); }); diff --git a/src/service/extension-script.js b/src/service/extension-script.js index f3274df8c043..b63a3ee15471 100644 --- a/src/service/extension-script.js +++ b/src/service/extension-script.js @@ -49,6 +49,25 @@ export function calculateExtensionScriptUrl( return `${base}/rtv/${rtv}/v0/${extensionId}${extensionVersion}${fileExtension}`; } +/** + * Calculate url for a file in the v0/ extension directory. + * @param {!Window} win The window + * @param {!Location} location The window's location + * @param {string} filename + * @param {boolean=} opt_isLocalDev + * @return {string} + */ +export function calculateExtensionFileUrl( + win, + location, + filename, + opt_isLocalDev +) { + const base = calculateScriptBaseUrl(location, opt_isLocalDev); + const rtv = getMode(win).rtvVersion; + return `${base}/rtv/${rtv}/v0/${filename}`; +} + /** * Calculate script url for an entry point. * If `opt_rtv` is true, returns the URL matching the current RTV. diff --git a/test/unit/test-extension-script.js b/test/unit/test-extension-script.js index 0a9c52206d07..db989e1b5f50 100644 --- a/test/unit/test-extension-script.js +++ b/test/unit/test-extension-script.js @@ -2,6 +2,7 @@ import {createElementWithAttributes} from '#core/dom'; import { calculateEntryPointScriptUrl, + calculateExtensionFileUrl, calculateExtensionScriptUrl, getExtensionScripts, parseExtensionUrl, @@ -323,6 +324,54 @@ describes.sandboxed('Module Extension Location', {}, () => { }); }); +describes.sandboxed('Extension File Location', {}, () => { + describe('get correct file location', () => { + beforeEach(() => { + // These functions must not rely on log for cases in SW. + resetLogConstructorForTesting(); + }); + + afterEach(() => { + initLogConstructor(); + window.__AMP_MODE = {}; + }); + + it('with local mode', () => { + window.__AMP_MODE = {rtvVersion: '123'}; + const script = calculateExtensionFileUrl( + window, + { + pathname: 'examples/ads.amp.html', + host: 'localhost:8000', + protocol: 'http:', + }, + 'some-file.json', + true + ); + expect(script).to.equal( + 'http://localhost:8000/dist/rtv/123/v0/some-file.json' + ); + }); + + it('with remote mode', () => { + window.__AMP_MODE = {rtvVersion: '123'}; + const script = calculateExtensionFileUrl( + window, + { + pathname: 'examples/ads.amp.html', + host: 'localhost:8000', + protocol: 'http:', + }, + 'some-file.json', + false + ); + expect(script).to.equal( + 'https://cdn.ampproject.org/rtv/123/v0/some-file.json' + ); + }); + }); +}); + describes.fakeWin('getExtensionScripts', {}, (env) => { let win, doc; From 0a4434ead0af41aa1d96ef896693ef10c52f38ae Mon Sep 17 00:00:00 2001 From: Jonathan Date: Tue, 5 Apr 2022 12:24:18 -0700 Subject: [PATCH 2/6] =?UTF-8?q?=E2=9C=85=20amp-story-shopping=20validation?= =?UTF-8?q?=20test=20to=20be=20re-added=20before=20launch=20(#36606)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * stories code lab work so far * updated shopping comonent to add vlidation and unit tests * updated stories codelab unit tests to work * finished the codelab * Added boiler plate with working linked css files and working test cases for amp story shopping tag, attachment, and config components * cleaned up a few files * added css file to make circleci happy * fixed formatting of owners file * added validator tests * Updated validation test * added descendat tags * added layout changes * added validtor tags for amp story shopping subcomponenets to story protoascii file * Added subcomponent elements to valdiation tests * fixed formatting * added mandatory ancestors * Added amp story page * added auto width for tag element * added tag and config to amp-story-protoascii * added child tags to correct location * fixed layout issues * updated validation tests * added mroe validation checks * Added increased test coverage for layouts * fixed unit test with correct alyout * added increased code coverage * added increased test coverage * fixed code coverage title bug with amp shopping tag unit test (needed an extra dash between the version number) * cleanup up unit tests * added shopping tag stuff * added requires usage method * cleaned up config files and test files * reconfigured protoascii * removed unnescessary config files * updated ownders files * fixed layout attribute so that publisher don't need ot specify * added fixed to css files * updated owners file * removed hasCSs line * removed css files * removed some non-validation files * removed even more non-validation files * added shopping validation code before launch * added link rel canonical * removed shopping config from protoascii * added required and optional attributes for amp story shoppiing tag and attachment * fixed AMP validation with correct protoascii fields * removed stray space * remove unused field * fixed validation formatiing * added container layout * updated attribute and child tags * added script config from auto ads * added test for script children * removed trialing whitespaces --- examples/amp-story/amp-story-shopping.html | 1 + .../amp-story-shopping-landscape.html | 1 + .../amp-story/amp-story-shopping-lang-de.html | 1 + .../amp-story/amp-story-shopping-rtl.html | 1 + .../test/validator-amp-story-shopping.html | 257 +++++++++++++++++ .../0.1/test/validator-amp-story-shopping.out | 258 ++++++++++++++++++ .../validator-amp-story-shopping.protoascii | 52 +++- ...lidator-amp-story-amp-experiment-error.out | 4 +- .../validator-amp-story-amp-twitter-error.out | 4 +- .../amp-story/validator-amp-story.protoascii | 2 + 10 files changed, 566 insertions(+), 15 deletions(-) create mode 100644 extensions/amp-story-shopping/0.1/test/validator-amp-story-shopping.html create mode 100644 extensions/amp-story-shopping/0.1/test/validator-amp-story-shopping.out diff --git a/examples/amp-story/amp-story-shopping.html b/examples/amp-story/amp-story-shopping.html index 9588e3939976..e139e368ceea 100644 --- a/examples/amp-story/amp-story-shopping.html +++ b/examples/amp-story/amp-story-shopping.html @@ -3,6 +3,7 @@ Shopping + diff --git a/examples/visual-tests/amp-story/amp-story-shopping-landscape.html b/examples/visual-tests/amp-story/amp-story-shopping-landscape.html index 5dd3036f42f9..5b65ec395853 100644 --- a/examples/visual-tests/amp-story/amp-story-shopping-landscape.html +++ b/examples/visual-tests/amp-story/amp-story-shopping-landscape.html @@ -3,6 +3,7 @@ Shopping + diff --git a/examples/visual-tests/amp-story/amp-story-shopping-lang-de.html b/examples/visual-tests/amp-story/amp-story-shopping-lang-de.html index 5db508b1a9cf..4f9f12e55f87 100644 --- a/examples/visual-tests/amp-story/amp-story-shopping-lang-de.html +++ b/examples/visual-tests/amp-story/amp-story-shopping-lang-de.html @@ -3,6 +3,7 @@ Shopping + diff --git a/examples/visual-tests/amp-story/amp-story-shopping-rtl.html b/examples/visual-tests/amp-story/amp-story-shopping-rtl.html index e763a50e41de..aab8f1158206 100644 --- a/examples/visual-tests/amp-story/amp-story-shopping-rtl.html +++ b/examples/visual-tests/amp-story/amp-story-shopping-rtl.html @@ -3,6 +3,7 @@ Shopping + diff --git a/extensions/amp-story-shopping/0.1/test/validator-amp-story-shopping.html b/extensions/amp-story-shopping/0.1/test/validator-amp-story-shopping.html new file mode 100644 index 000000000000..17e600a8806c --- /dev/null +++ b/extensions/amp-story-shopping/0.1/test/validator-amp-story-shopping.html @@ -0,0 +1,257 @@ + + + + + Shopping + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/extensions/amp-story-shopping/0.1/test/validator-amp-story-shopping.out b/extensions/amp-story-shopping/0.1/test/validator-amp-story-shopping.out new file mode 100644 index 000000000000..7b2ce6513d3d --- /dev/null +++ b/extensions/amp-story-shopping/0.1/test/validator-amp-story-shopping.out @@ -0,0 +1,258 @@ +PASS +| +| +| +| +| Shopping +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| +| diff --git a/extensions/amp-story-shopping/validator-amp-story-shopping.protoascii b/extensions/amp-story-shopping/validator-amp-story-shopping.protoascii index ca7752c24d96..9c42003f3446 100644 --- a/extensions/amp-story-shopping/validator-amp-story-shopping.protoascii +++ b/extensions/amp-story-shopping/validator-amp-story-shopping.protoascii @@ -14,29 +14,59 @@ html_format: AMP tag_name: "AMP-STORY-SHOPPING-TAG" requires_extension: "amp-story-shopping" mandatory_ancestor: "AMP-STORY-GRID-LAYER" - attr_lists: "extended-amp-global" spec_url: "https://amp.dev/documentation/components/amp-story-shopping/" amp_layout: { supported_layouts: CONTAINER } + attrs: { + name: "data-product-id" + mandatory: true + } } -tags: { # +tags: { # html_format: AMP - tag_name: "AMP-STORY-SHOPPING-CONFIG" + tag_name: "AMP-STORY-SHOPPING-ATTACHMENT" requires_extension: "amp-story-shopping" - attr_lists: "extended-amp-global" + mandatory_last_child: true spec_url: "https://amp.dev/documentation/components/amp-story-shopping/" + attrs: { + name: "src" + value_url: { + protocol: "https" + allow_relative: true + } + } + attrs: { + name: "theme" + value: "dark" + value: "light" + } + child_tags: { + child_tag_name_oneof: "SCRIPT" + mandatory_min_num_child_tags: 1 + } amp_layout: { - supported_layouts: NODISPLAY + supported_layouts: CONTAINER } } -tags: { # +tags: { # amp-story-shopping-attachment html_format: AMP - tag_name: "AMP-STORY-SHOPPING-ATTACHMENT" + tag_name: "SCRIPT" + spec_name: "amp-story-shopping-attachment config script" + mandatory_parent: "AMP-STORY-SHOPPING-ATTACHMENT" requires_extension: "amp-story-shopping" - attr_lists: "extended-amp-global" - spec_url: "https://amp.dev/documentation/components/amp-story-shopping/" - amp_layout: { - supported_layouts: NODISPLAY + attrs: { + name: "type" + mandatory: true + value_casei: "application/json" + dispatch_key: NAME_VALUE_PARENT_DISPATCH + } + attr_lists: "nonce-attr" + cdata: { + disallowed_cdata_regex: { + regex: " | >> ^~~~~~~~~ -amp-story/1.0/test/validator-amp-story-amp-experiment-error.html:49:6 Tag 'amp-experiment' is disallowed as child of tag 'amp-story-page'. Child tag must be one of ['amp-analytics', 'amp-pixel', 'amp-story-animation', 'amp-story-auto-analytics', 'amp-story-cta-layer', 'amp-story-grid-layer', 'amp-story-page-attachment', 'amp-story-page-outlink']. (see https://amp.dev/documentation/components/amp-story) +amp-story/1.0/test/validator-amp-story-amp-experiment-error.html:49:6 Tag 'amp-experiment' is disallowed as child of tag 'amp-story-page'. Child tag must be one of ['amp-analytics', 'amp-pixel', 'amp-story-animation', 'amp-story-auto-analytics', 'amp-story-cta-layer', 'amp-story-grid-layer', 'amp-story-page-attachment', 'amp-story-page-outlink', 'amp-story-shopping-attachment']. (see https://amp.dev/documentation/components/amp-story) |