From 4e31c2d23350011059c535489c06c2f8c4e24108 Mon Sep 17 00:00:00 2001 From: Alan Orozco Date: Thu, 6 May 2021 10:36:47 -0700 Subject: [PATCH] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20=F0=9F=93=96=20Global=20de?= =?UTF-8?q?p-check=20rule=20for=20Bento=20video=20(#34252)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We can more simply allow `extensions/**` files to import specific files from `amp-video/1.0`. It should be ok in any case. --- build-system/test-configs/dep-check-config.js | 22 ++++------- contributing/building-a-bento-video-player.md | 37 ------------------- 2 files changed, 7 insertions(+), 52 deletions(-) diff --git a/build-system/test-configs/dep-check-config.js b/build-system/test-configs/dep-check-config.js index 2fabcc703647..1f70148dd53e 100644 --- a/build-system/test-configs/dep-check-config.js +++ b/build-system/test-configs/dep-check-config.js @@ -245,23 +245,15 @@ exports.rules = [ 'extensions/amp-facebook-comments/0.1/amp-facebook-comments.js->extensions/amp-facebook/0.1/facebook-loader.js', 'extensions/amp-facebook-comments/1.0/amp-facebook-comments.js->extensions/amp-facebook/0.1/facebook-loader.js', - // Bento VideoIframe, amp-video-iframe - 'extensions/amp-video-iframe/1.0/base-element.js->extensions/amp-video/1.0/base-element.js', - 'extensions/amp-video-iframe/1.0/component.js->extensions/amp-video/1.0/video-iframe.js', - // Shared definition of the iframe integration API - 'extensions/amp-video-iframe/0.1/amp-video-iframe.js->extensions/amp-video-iframe/amp-video-iframe-api.js', - 'extensions/amp-video-iframe/1.0/amp-video-iframe.js->extensions/amp-video-iframe/amp-video-iframe-api.js', + // VideoBaseElement, VideoIframe and VideoWrapper are meant to be shared. + 'extensions/**->extensions/amp-video/1.0/base-element.js', + 'extensions/**->extensions/amp-video/1.0/video-iframe.js', - // Bento Vimeo, amp-vimeo - 'extensions/amp-vimeo/1.0/base-element.js->extensions/amp-video/1.0/base-element.js', - 'extensions/amp-vimeo/1.0/component.js->extensions/amp-video/1.0/video-iframe.js', - // Shared definition of Vimeo API - 'extensions/amp-vimeo/0.1/amp-vimeo.js->extensions/amp-vimeo/vimeo-api.js', - 'extensions/amp-vimeo/1.0/component.js->extensions/amp-vimeo/vimeo-api.js', + // versions share this message API definition. + 'extensions/amp-video-iframe/**->extensions/amp-video-iframe/amp-video-iframe-api.js', - // Bento Youtube, amp-youtube - 'extensions/amp-youtube/1.0/base-element.js->extensions/amp-video/1.0/base-element.js', - 'extensions/amp-youtube/1.0/component.js->extensions/amp-video/1.0/video-iframe.js', + // versions share this message API definition. + 'extensions/amp-vimeo/**->extensions/amp-vimeo/vimeo-api.js', // Amp geo in group enum 'extensions/amp-a4a/0.1/amp-a4a.js->extensions/amp-geo/0.1/amp-geo-in-group.js', diff --git a/contributing/building-a-bento-video-player.md b/contributing/building-a-bento-video-player.md index 859b5c8b072e..00121191d5a2 100644 --- a/contributing/building-a-bento-video-player.md +++ b/contributing/building-a-bento-video-player.md @@ -101,21 +101,6 @@ export class BaseElement extends VideoBaseElement {} This enables support for AMP actions and analytics, once we map attributes to their prop counterparts in `BaseElement['props']`, and we implement the Preact component. -**Adding this import statement will cause CI to fail**. You should solve this by explicitly allowing the cross-extension import, which is ok for this target file. We add to **`build-system/test-configs/dep-check-config.js`**: - -```diff -{ - // Extensions can't depend on other extensions. - filesMatching: 'extensions/**/*.js', - mustNotDependOn: 'extensions/**/*.js', - allowlist: [ - ... -+ // Bento MyFantasticPlayer, amp-my-fantastic-player -+ 'extensions/amp-my-fantastic-player/1.0/base-element.js->extensions/amp-video/1.0/base-element.js', - ], -}, -``` - ### Pre-upgrade CSS Bento components must specify certain layout properties in order to prevent [Cumulative Layout Shift (CLS)](https://web.dev/cls). Video extensions must include the following in the generated **`amp-fantastic-player.css`**: @@ -254,17 +239,6 @@ function FantasticPlayerWithRef({...rest}, ref) { We're rendering an iframe that always loads `https://example.com/fantastic`, but we'll specify a dynamic URL later. Likewise, we'll need to define implementations for the communication functions `makeMethodMessage` and `onMessage`. -You should list one more allowed cross-extension import for `VideoIframe`. Following the previous location on **`dep-check-config.js`**, we add: - -```diff - allowlist: [ - ... - // Bento MyFantasticPlayer, amp-my-fantastic-player - 'extensions/amp-my-fantastic-player/1.0/base-element.js->extensions/amp-video/1.0/base-element.js', -+ 'extensions/amp-my-fantastic-player/1.0/component.js->extensions/amp-video/1.0/video-iframe.js', - ], -``` - #### `src` You may use props to construct the `src`, like using a `videoId` to load `https://example.com/fantastic/${videoId}/`. @@ -436,17 +410,6 @@ function FantasticPlayerWithRef({...rest}, ref) { We're specifying `"video"` as the element to render, which is also the default. We'll later change this into our own component implementation. -You should list one more allowed cross-extension import for `VideoWrapper`. Following the previous location on **`dep-check-config.js`**, we add: - -```diff - allowlist: [ - ... - // Bento MyFantasticPlayer, amp-my-fantastic-player - 'extensions/amp-my-fantastic-player/1.0/base-element.js->extensions/amp-video/1.0/base-element.js', -+ 'extensions/amp-my-fantastic-player/1.0/component.js->extensions/amp-video/1.0/video-wrapper.js', - ], -``` - #### Specifying `component` The `VideoWrapper` must interact with an element that implements the `HTMLMediaInterface` like `