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.

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

Use ES2015 destructuring assignment to swap variables #4674

Closed
wants to merge 1 commit into from

Conversation

phanan
Copy link
Member

@phanan phanan commented Jan 7, 2017

This commit uses ES2015 destructuring assignment to swap two variables instead of using a tmp var. Flow has an open issue (facebook/flow#183) with this feature, though, so // $FlowFixMe is used to temporarily suppress the error.

This commit uses ES2015 destructuring assignment to swap two variables
instead of using a tmp var. Flow has an open issue (facebook/flow#183)
with this feature, though, so // $FlowFixMe is used to temporarily
suppress the error.
this.deps = this.newDeps
this.newDeps = tmp
// $FlowFixMe
[this.depIds, this.newDepIds] = [this.newDepIds, this.depIds]
Copy link
Member

Choose a reason for hiding this comment

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

Maybe leading semicolon is better for []? Since the change is packed in one line so it is harder for one to forget adding semicolon.

Copy link
Member Author

Choose a reason for hiding this comment

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

If they forget, the lint/test process will certainly remind them, right?

@yyx990803
Copy link
Member

Checked the output in Buble and it uses an intermediate Array for the purpose: https://buble.surge.sh/#%5Bthis.depIds%2C%20this.newDepIds%5D%20%3D%20%5Bthis.newDepIds%2C%20this.depIds%5D - which leads to more allocation than necessary - and this part is pretty perf-sensitive.

P.S. personally I'm not really a fan of this syntax's readability :/

@yyx990803 yyx990803 closed this Jan 12, 2017
@phanan
Copy link
Member Author

phanan commented Jan 13, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants