Skip to content

🏗 Allow array destructuring on preact hooks#26901

Merged
jridgewell merged 2 commits intoampproject:masterfrom
jridgewell:babel-preact
Feb 26, 2020
Merged

🏗 Allow array destructuring on preact hooks#26901
jridgewell merged 2 commits intoampproject:masterfrom
jridgewell:babel-preact

Conversation

@jridgewell
Copy link
Copy Markdown
Contributor

@jridgewell jridgewell commented Feb 21, 2020

No description provided.

@jridgewell jridgewell requested a review from dvoytenko February 21, 2020 00:37
@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Feb 21, 2020 •
edited
Loading

Hey @erwinmombay, these files were changed:

  • build-system/babel-plugins/babel-plugin-transform-simple-array-destructure/index.js
  • build-system/babel-plugins/babel-plugin-transform-simple-array-destructure/test/fixtures/transform/deep-array/input.js
  • build-system/babel-plugins/babel-plugin-transform-simple-array-destructure/test/fixtures/transform/deep-array/options.json
  • build-system/babel-plugins/babel-plugin-transform-simple-array-destructure/test/fixtures/transform/deep/input.js
  • build-system/babel-plugins/babel-plugin-transform-simple-array-destructure/test/fixtures/transform/deep/options.json
  • build-system/babel-plugins/babel-plugin-transform-simple-array-destructure/test/fixtures/transform/param/input.js
  • build-system/babel-plugins/babel-plugin-transform-simple-array-destructure/test/fixtures/transform/param/options.json
  • build-system/babel-plugins/babel-plugin-transform-simple-array-destructure/test/fixtures/transform/skip-element/input.js
  • build-system/babel-plugins/babel-plugin-transform-simple-array-destructure/test/fixtures/transform/skip-element/options.json
  • build-system/babel-plugins/babel-plugin-transform-simple-array-destructure/test/fixtures/transform/skip-element/output.js
  • build-system/babel-plugins/babel-plugin-transform-simple-array-destructure/test/fixtures/transform/variable-declarations/input.js
  • build-system/babel-plugins/babel-plugin-transform-simple-array-destructure/test/fixtures/transform/variable-declarations/options.json
  • build-system/babel-plugins/babel-plugin-transform-simple-array-destructure/test/fixtures/transform/variable-declarations/output.js
  • build-system/babel-plugins/babel-plugin-transform-simple-array-destructure/test/index.js
  • build-system/eslint-rules/no-array-destructuring.js

samouri
samouri reviewed Feb 21, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@samouri samouri left a comment •
edited
Loading

Choose a reason for hiding this comment

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

How does this handle the swap and default value cases?

swap

let a = 1;
let b = 3;
[a, b] = [b, a];

default values

let a, b;

[a=5, b=7] = [1];

I think it is okay if we don't, since we are really only trying to support a single pattern.

@jridgewell
Copy link
Copy Markdown
Contributor Author

jridgewell commented Feb 21, 2020

They're not approved by the linter, so it'll yell at you.

samouri
samouri approved these changes Feb 21, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@samouri samouri left a comment

Choose a reason for hiding this comment

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

LGTM. Can you update the PR description with at least an example before/after?

kristoferbaxter
kristoferbaxter reviewed Feb 21, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@kristoferbaxter kristoferbaxter left a comment

Choose a reason for hiding this comment

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

Have to run, but wanted to ensure these comments were available.

...babel-plugin-transform-simple-array-destructure/test/fixtures/transform/deep-array/output.js Outdated
*/
const _temp = document,
_temp2 = _temp[0],
first = _temp2[0];
Copy link
Copy Markdown
Contributor

@kristoferbaxter kristoferbaxter Feb 21, 2020

Choose a reason for hiding this comment

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

Interestingly for this deep case, the output is slightly smaller than with object destructure.

let o=document[0][0];console.log(o) versus let{0:o}=document,{0:n}=o;console.log(n)

Copy link
Copy Markdown
Contributor Author

@jridgewell jridgewell Feb 21, 2020

Choose a reason for hiding this comment

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

Oh, that reminds me that I wanted to implement this with object destructure. Lol.

...bel-plugin-transform-simple-array-destructure/test/fixtures/transform/skip-element/output.js
const _temp2 = document,
x = _temp2[0],
z = _temp2[2];
}
Copy link
Copy Markdown
Contributor

@kristoferbaxter kristoferbaxter Feb 21, 2020

Choose a reason for hiding this comment

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

Looks like terser undoes some of this
let[,n]=document,[o,,c]=document

Copy link
Copy Markdown
Contributor Author

@jridgewell jridgewell Feb 21, 2020

Choose a reason for hiding this comment

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

Under what options?

Copy link
Copy Markdown
Contributor

@kristoferbaxter kristoferbaxter Feb 21, 2020

Choose a reason for hiding this comment

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

Pulled the options from our single pass configuration. But cannot reproduce now.

Looking into this again.

kristoferbaxter
kristoferbaxter approved these changes Feb 21, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@kristoferbaxter kristoferbaxter left a comment

Choose a reason for hiding this comment

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

Cannot reproduce the terser unwind. With Object destructuring, it looks good to me.

@jridgewell jridgewell merged commit 2454849 into ampproject:master Feb 26, 2020
@jridgewell jridgewell mentioned this pull request Feb 26, 2020
Closed
@jridgewell jridgewell deleted the babel-preact branch February 27, 2020 05:18
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

3 more reviewers

@kristoferbaxter kristoferbaxter kristoferbaxter approved these changes

@dvoytenko dvoytenko dvoytenko approved these changes

@samouri samouri samouri approved these changes

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.

6 participants

@jridgewell @kristoferbaxter @dvoytenko @samouri @googlebot @ampprojectbot