Skip to content

🏗 Transform aliased configured components#26541

Merged
alanorozco merged 36 commits intoampproject:masterfrom
alanorozco:define-video-element
Feb 26, 2020
Merged

🏗 Transform aliased configured components#26541
alanorozco merged 36 commits intoampproject:masterfrom
alanorozco:define-video-element

Conversation

@alanorozco
Copy link
Copy Markdown
Member

@alanorozco alanorozco commented Jan 29, 2020 •
edited
Loading

Partial for #26572.

Transformer

transform-inline-configure-component finds configureComponent(MyConstructor, {foo: 'bar'}) calls and:

  1. Inlines imported MyConstructor in current scope.

  2. Replaces *.*.STATIC_CONG_.foo access with its value as defined in config object {foo: 'bar'}.

  3. Replaces configureComponent() call with inlined MyConstructor's id.

For runtime use of configureComponent like:

import {MyConstructor} from './my-ctor';
const TAG = 'amp-foo';
const MyWrappedConstructor = configureComponent(
  MyConstructor,
  {TAG, foo: 'foo'}
);

Composed class gets aliased, hoisted and its configuration replaced inline:

import {something} from './imported-by-constructor';

class MyConstructor {
  constructor() {
    // references to config object are replaced here, from:
    // `console.log(this.STATIC_CONFIG_.foo)`
    console.log(_foo);

    // ids in global scope are kept:
    const TAG = TAG;

    // unset properties are replaced with undefined, so this would
    // get minified
    const something = undefined || 'default value';

    // destructuring keeps only the default value when prop undefined:
    // `const {bar = 'default value'} = this.STATIC_CONFIG_`
    const bar = 'default value';
  }
}

const _foo = 'foo';

const TAG = 'amp-foo';
const MyWrappedConstructor = MyConstructor;

@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Jan 29, 2020 •
edited
Loading

Hey @erwinmombay, these files were changed:

  • build-system/babel-plugins/babel-plugin-transform-inline-configure-component/index.js
  • build-system/babel-plugins/babel-plugin-transform-inline-configure-component/test/fixtures/inline-decl-extensions/destructuring/input-base-class.js
  • build-system/babel-plugins/babel-plugin-transform-inline-configure-component/test/fixtures/inline-decl-extensions/destructuring/input.js
  • build-system/babel-plugins/babel-plugin-transform-inline-configure-component/test/fixtures/inline-decl-extensions/destructuring/options.json
  • build-system/babel-plugins/babel-plugin-transform-inline-configure-component/test/fixtures/inline-decl-extensions/destructuring/output.mjs
  • build-system/babel-plugins/babel-plugin-transform-inline-configure-component/test/fixtures/inline-decl-extensions/direct-access/input-base-class.js
  • build-system/babel-plugins/babel-plugin-transform-inline-configure-component/test/fixtures/inline-decl-extensions/direct-access/input.js
  • build-system/babel-plugins/babel-plugin-transform-inline-configure-component/test/fixtures/inline-decl-extensions/direct-access/options.json
  • build-system/babel-plugins/babel-plugin-transform-inline-configure-component/test/fixtures/inline-decl-extensions/direct-access/output.mjs
  • build-system/babel-plugins/babel-plugin-transform-inline-configure-component/test/fixtures/inline-decl-extensions/relative-imports/input-nested-directory/input-base-class.js
  • build-system/babel-plugins/babel-plugin-transform-inline-configure-component/test/fixtures/inline-decl-extensions/relative-imports/input.js
  • build-system/babel-plugins/babel-plugin-transform-inline-configure-component/test/fixtures/inline-decl-extensions/relative-imports/options.json
  • build-system/babel-plugins/babel-plugin-transform-inline-configure-component/test/fixtures/inline-decl-extensions/relative-imports/output.mjs
  • build-system/babel-plugins/babel-plugin-transform-inline-configure-component/test/fixtures/inline-decl-extensions/remove-assignment/input-base-class.js
  • build-system/babel-plugins/babel-plugin-transform-inline-configure-component/test/fixtures/inline-decl-extensions/remove-assignment/input.js
  • build-system/babel-plugins/babel-plugin-transform-inline-configure-component/test/fixtures/inline-decl-extensions/remove-assignment/options.json
  • build-system/babel-plugins/babel-plugin-transform-inline-configure-component/test/fixtures/inline-decl-extensions/remove-assignment/output.mjs
  • build-system/babel-plugins/babel-plugin-transform-inline-configure-component/test/index.js

@alanorozco alanorozco force-pushed the define-video-element branch 2 times, most recently from 3fc5316 to cbc0b0b Compare January 29, 2020 18:43
@alanorozco alanorozco force-pushed the define-video-element branch from cbc0b0b to 46f1d72 Compare January 29, 2020 18:45
@ampproject ampproject deleted a comment from lgtm-com bot Jan 29, 2020
@alanorozco alanorozco mentioned this pull request Jan 31, 2020
Closed
@alanorozco alanorozco changed the title ♻️🏗 Aliased declarative video players 🏗 Aliased configured components Feb 3, 2020
@alanorozco alanorozco marked this pull request as ready for review February 3, 2020 16:54
@amp-owners-bot amp-owners-bot bot requested a review from jridgewell February 3, 2020 16:54
@alanorozco alanorozco force-pushed the define-video-element branch from fa772e4 to 3069003 Compare February 4, 2020 16:41
kevinkimball
kevinkimball reviewed Feb 11, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@kevinkimball kevinkimball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems promising if we are going to go this way.

erwinmombay
erwinmombay reviewed Feb 13, 2020
View reviewed changes
build-system/babel-plugins/babel-plugin-transform-inline-configure-component/index.js Outdated
const currentDirname = dirname(file.opts.filename);
const importedModule = join(currentDirname, importPath);

// TODO(alanorozco): sourcemaps
Copy link
Copy Markdown
Member

@erwinmombay erwinmombay Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a tracking issue for this, i believe you're possibly going to need resorcery and terser for this

Copy link
Copy Markdown
Member Author

@alanorozco alanorozco Feb 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added issue. Why terser? What am I missing :)

Copy link
Copy Markdown
Member

@erwinmombay erwinmombay Feb 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/ampproject/amphtml/pull/26779/files#diff-321517b76b9f18ac62dff6aa46ec79b1R102-R135

https://github.com/ampproject/amphtml/blob/master/build-system/compile/single-pass.js#L771-L803

i'm not sure but @kristoferbaxter and @jridgewell used it in these to fix up the source maps in conjunction with resorcery (they can probably explain)

@alanorozco alanorozco mentioned this pull request Feb 24, 2020
Open
@alanorozco alanorozco changed the title 🏗 Aliased configured components 🏗 Transform aliased configured components Feb 26, 2020
@alanorozco alanorozco merged commit 6e49702 into ampproject:master Feb 26, 2020
@alanorozco alanorozco deleted the define-video-element branch February 26, 2020 21:06
robinvanopstal added a commit to jungvonmatt/amphtml that referenced this pull request Feb 27, 2020
@robinvanopstal
Merge branch 'master' into component-intro-tables
4d2af31 
* master: (54 commits)
  inabox-resources: Minor test improvement (ampproject#26916)
  DocInfo: replace metaTags with viewport in API (ampproject#26687)
  🐛 SwG now uses AMP sendBeacon interface (ampproject#26970)
  🏗 Allow array destructuring on preact hooks (ampproject#26901)
  Gulp Dep Check: fail on unused entries (ampproject#26981)
  Update no-import lint rule to forbid sub-paths (ampproject#26531)
  🐛 amp-ad type blade - fix bladeOnLoad callback (ampproject#26627)
  📖 Clarify when max-age is required (ampproject#26956)
  ♻️ Consolidate players as .i-amphtml-media-component (ampproject#26967)
  Add Preact Enzyme tests (ampproject#26529)
  Fixes `update_tests` flag on `gulp validator` (ampproject#26965)
  📦 Update dependency google-closure-library to v20200224 (ampproject#26986)
  🏗 Transform aliased configured components (ampproject#26541)
  ✨ InaboxResources: Observe intersections for some elements' viewportCallbacks (ampproject#26942)
  variable substitutions: Support allowlist lookup in AmpDocShadow (ampproject#26731)
  cl/297197875 Revision bump for ampproject#26877 (ampproject#26982)
  Json fix (ampproject#26971)
  📦 Update dependency mocha to v7.1.0 (ampproject#26976)
  Add documentation for amp-access-scroll (ampproject#26782)
  make controls always shown in amp for email (ampproject#25714)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erwinmombay erwinmombay erwinmombay approved these changes

@jridgewell jridgewell Awaiting requested review from jridgewell

@kristoferbaxter kristoferbaxter Awaiting requested review from kristoferbaxter

+1 more reviewer

@kevinkimball kevinkimball kevinkimball left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes PR use: In Beta / Experimental PR use: In Stable

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@alanorozco @erwinmombay @kevinkimball @googlebot @ampprojectbot