Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

🏗Prefer destructuring objects #15204

Merged
merged 16 commits into from
May 17, 2018
Merged

🏗Prefer destructuring objects #15204

merged 16 commits into from
May 17, 2018

Conversation

jridgewell
Copy link
Contributor

@jridgewell jridgewell commented May 10, 2018 •
edited
Loading

This adds a (auto fixable!) lint rule to prefer destructing declarations instead of repeating the name twice:

// Bad
const name = obj.field.name;

// Good
const {name} = obj.field;

Additionally, it adds a (also auto fixable!) lint rule to combine multiple destructures of the same object, further saving space:

// Bad
const a = obj.field.a;
const {b} = obj.field;
const rename = obj.field.c;
const {d, e} = obj.field;

// Good
const {a, b, c: rename, d, e} = obj.field;

@ghost
Copy link

ghost commented May 10, 2018

This pull request fixes 1 alert when merging f73785a into f7bb404 - view on lgtm.com

fixed alerts:

  • 1 for Reflected cross-site scripting

Comment posted by lgtm.com

dreamofabear
dreamofabear reviewed May 11, 2018
View reviewed changes
build-system/app.js Outdated
@@ -342,7 +342,7 @@ app.use('/examples/live-list-update(-reverse)?.amp.html', (req, res, next) => {
// When we already have state in memory and user refreshes page, we flush
// the dom we maintain on the server.
if (!('amp_latest_update_time' in req.query) && liveListDoc) {
let outerHTML = liveListDoc.documentElement./*OK*/outerHTML;
Copy link

Choose a reason for hiding this comment

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

Does this circumvent our property accessor checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Damn, I'll add an exception for this.

build-system/eslint-rules/no-deep-destructuring.js Outdated
*/
'use strict';

module.exports = function(context) {
Copy link

Choose a reason for hiding this comment

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

Can you provide an explanation with an example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

build-system/tasks/process-github-issues.js
milestone,
assignee,
'pull_request': pullRequest,
'updated_at': issueLastUpdate,
Copy link

Choose a reason for hiding this comment

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

Huh, didn't know you could do that.

extensions/amp-access/0.1/iframe-api/messenger.js Outdated
if (!data || data['sentinel'] != SENTINEL) {
return;
}
const origin = /** @type {string} */ (event.origin);
Copy link

Choose a reason for hiding this comment

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

Lost type information?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A few don't seem necessary.

@ghost
Copy link

ghost commented May 11, 2018

This pull request fixes 1 alert when merging 15577df into f7bb404 - view on lgtm.com

fixed alerts:

  • 1 for Reflected cross-site scripting

Comment posted by lgtm.com

@ghost
Copy link

ghost commented May 11, 2018

This pull request fixes 1 alert when merging 721d573 into f7bb404 - view on lgtm.com

fixed alerts:

  • 1 for Reflected cross-site scripting

Comment posted by lgtm.com

@ghost
Copy link

ghost commented May 11, 2018

This pull request fixes 1 alert when merging bb5d2e9 into 291df9e - view on lgtm.com

fixed alerts:

  • 1 for Reflected cross-site scripting

Comment posted by lgtm.com

@ghost
Copy link

ghost commented May 14, 2018

This pull request fixes 1 alert when merging d862981 into 91e7a6a - view on lgtm.com

fixed alerts:

  • 1 for Reflected cross-site scripting

Comment posted by lgtm.com

@jridgewell
Copy link
Contributor Author

Ping @choumx, needs approval.

@ghost
Copy link

ghost commented May 15, 2018

This pull request fixes 1 alert when merging 75ec660 into 9cb1201 - view on lgtm.com

fixed alerts:

  • 1 for Reflected cross-site scripting

Comment posted by lgtm.com

@rsimha
Copy link
Contributor

rsimha commented May 15, 2018

With this code, you should be able to bypass strict linting:

amphtml/build-system/tasks/lint.js

Lines 200 to 202 in 32ec5af

} else if (jsFiles.length > filesInARefactorPr) {
// This is probably a refactor, don't enable strict mode.
setFilesToLint(jsFiles);

@jridgewell
Copy link
Contributor Author

Ping @choumx

@ghost
Copy link

ghost commented May 15, 2018

This pull request fixes 1 alert when merging c95da6a into 4058693 - view on lgtm.com

fixed alerts:

  • 1 for Reflected cross-site scripting

Comment posted by lgtm.com

@ghost
Copy link

ghost commented May 15, 2018

This pull request fixes 1 alert when merging 9e53e1b into 80914d9 - view on lgtm.com

fixed alerts:

  • 1 for Reflected cross-site scripting

Comment posted by lgtm.com

@ghost
Copy link

ghost commented May 17, 2018

This pull request fixes 1 alert when merging 7352832 into 5675ddc - view on lgtm.com

fixed alerts:

  • 1 for Reflected cross-site scripting

Comment posted by lgtm.com

@ghost
Copy link

ghost commented May 17, 2018

This pull request fixes 1 alert when merging 19c45db into 7a6e36d - view on lgtm.com

fixed alerts:

  • 1 for Reflected cross-site scripting

Comment posted by lgtm.com

rsimha
rsimha reviewed May 17, 2018
View reviewed changes
build-system/eslint-rules/no-deep-destructuring.js
@@ -0,0 +1,35 @@
/**
* Copyright 2016 The AMP HTML Authors. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm gonna fix this in a follow up.

@jridgewell jridgewell merged commit acc1927 into ampproject:master May 17, 2018
@jridgewell jridgewell deleted the prefer-destructuring branch May 17, 2018 18:00
@jridgewell jridgewell mentioned this pull request May 17, 2018
Closed
@aghassemi aghassemi mentioned this pull request May 18, 2018
Merged
@Enriqe Enriqe mentioned this pull request Jun 28, 2018
Merged
@ghost ghost mentioned this pull request Jul 23, 2018
Closed
@ampprojectbot ampprojectbot mentioned this pull request Oct 20, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rsimha rsimha rsimha left review comments

@dreamofabear dreamofabear dreamofabear approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jridgewell @rsimha @dreamofabear @googlebot