Skip to content

Commit

Permalink
♻️ 📖 Global dep-check rule for Bento video (#34252)
Browse files Browse the repository at this point in the history
We can more simply allow `extensions/**` files to import specific files from `amp-video/1.0`. It should be ok in any case.
  • Loading branch information
alanorozco committed May 6, 2021
1 parent 51c0542 commit 4e31c2d
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 52 deletions.
22 changes: 7 additions & 15 deletions build-system/test-configs/dep-check-config.js
Expand Up @@ -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',
// <amp-video-iframe> 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',
// <amp-vimeo> 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',
Expand Down
37 changes: 0 additions & 37 deletions contributing/building-a-bento-video-player.md
Expand Up @@ -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`**:
Expand Down Expand Up @@ -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}/`.
Expand Down Expand Up @@ -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 `<video>`, or a Preact component that emulates the same interface.
Expand Down

0 comments on commit 4e31c2d

Please sign in to comment.