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

Evaluate computed class props only once #6466

Merged
merged 1 commit into from Oct 13, 2017

Conversation

@Qantas94Heavy
Copy link
Member

Qantas94Heavy commented Oct 12, 2017

Q                       A
Fixed Issues? Fixes #6240
Patch: Bug Fix? yes
Major: Breaking Change? no
Minor: New Feature? no
Tests Added + Pass? Yes
Documentation PR no
Any Dependency Changes? no

Previously, computed class properties would be evaluated every time a
new instance of the class was created. This means the property name
may have changed between different instances, as well as potential side
effects.

This commit fixes this by storing the computed value in a separate variable.

@Qantas94Heavy Qantas94Heavy changed the title Evaluate computed class props only once (#6240) Evaluate computed class props only once Oct 12, 2017
@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Oct 12, 2017

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/5284/

@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Oct 12, 2017

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/5264/

@Qantas94Heavy Qantas94Heavy force-pushed the Qantas94Heavy:class-props-eval-once branch 2 times, most recently from 6d57687 to e981078 Oct 12, 2017
Copy link
Member

nicolo-ribaudo left a comment

LGTM, I only left a suggestion to make the output easier to read for an human.

// Make sure computed property names are only evaluated once (upon
// class definition).
if (propNode.computed) {
const ident = path.scope.generateUidIdentifier("computed");

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Oct 12, 2017

Member

Here you could use path.scope.generateUidIdentifierBasedOnNode(propNode.value, "computed"), which generates better names.

e.g. class A { [x.y] = 2; } should become something like:

var A = function A() {
  _classCallCheck(this, A);

  Object.defineProperty(this, _x$y, {
    configurable: true,
    enumerable: true,
    writable: true,
    value: 2
  });
};

var _x$y = x;

This comment has been minimized.

Copy link
@Qantas94Heavy

Qantas94Heavy Oct 13, 2017

Author Member

Ah, didn't know about that. Thanks!

Copy link

littledan left a comment

Thanks for this great fix!

Previously, computed class properties would be evaluated every time a
new instance of the class was created. This means the property name
may have changed between different instances, as well as potential side
effects.

This commit fixes this by storing the computed value in a separate
variable.
@Qantas94Heavy Qantas94Heavy force-pushed the Qantas94Heavy:class-props-eval-once branch from e981078 to 24762fe Oct 13, 2017
@nicolo-ribaudo nicolo-ribaudo merged commit 5f285c1 into babel:master Oct 13, 2017
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 87.31% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Oct 13, 2017

Thank you!

t.variableDeclarator(ident, propNode.key),
]),
);
propNode.key = ident;

This comment has been minimized.

Copy link
@jridgewell

jridgewell Oct 19, 2017

Member

This needs to be cloned.

@lock lock bot added the outdated label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.