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

Fixed computed keys for class expression #10029

Merged
merged 6 commits into from May 28, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 9 additions & 3 deletions packages/babel-helper-create-class-features-plugin/src/misc.js
Expand Up @@ -98,10 +98,16 @@ export function extractComputedKeys(ref, path, computedPaths, file) {
const ident = path.scope.generateUidIdentifierBasedOnNode(
computedNode.key,
);
// Declaring in the same block scope
// Ref: https://github.com/babel/babel/pull/10029/files#diff-fbbdd83e7a9c998721c1484529c2ce92
path.scope.push({
id: ident,
kind: "let",
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any advantage in using let here? Usually we try to generate an output as low-level as possible, if it isn't harder to do.

Copy link
Member Author

@tanhauhau tanhauhau May 27, 2019

Choose a reason for hiding this comment

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

It is for this scenario: microsoft/TypeScript#27864

if it is set as var, it will behave the same as pointed out in the issue link above, which will break this newly added test case (https://github.com/babel/babel/pull/10029/files#diff-fbbdd83e7a9c998721c1484529c2ce92)

maybe it should be determined by whether the computedKeys original declaration kind, to preserve the original scoping intention?

Copy link
Member

Choose a reason for hiding this comment

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

Oh ok; it's ok like this then.

Copy link
Member

Choose a reason for hiding this comment

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

Seems like adding a comment explaining this might be useful?

});
declarations.push(
t.variableDeclaration("var", [
t.variableDeclarator(ident, computedNode.key),
]),
t.expressionStatement(
t.assignmentExpression("=", t.cloneNode(ident), computedNode.key),
),
);
computedNode.key = t.cloneNode(ident);
}
Expand Down
Expand Up @@ -16,11 +16,12 @@ function (_Hello) {
babelHelpers.inherits(Outer, _Hello);

function Outer() {
let _this2;

var _this;

babelHelpers.classCallCheck(this, Outer);

var _this2 = _this = babelHelpers.possibleConstructorReturn(this, babelHelpers.getPrototypeOf(Outer).call(this));
_this2 = _this = babelHelpers.possibleConstructorReturn(this, babelHelpers.getPrototypeOf(Outer).call(this));

let Inner = function Inner() {
babelHelpers.classCallCheck(this, Inner);
Expand Down
Expand Up @@ -22,12 +22,13 @@ function (_Hello) {
babelHelpers.inherits(Outer, _Hello);

function Outer() {
let _babelHelpers$get$cal;

var _this;

babelHelpers.classCallCheck(this, Outer);
_this = babelHelpers.possibleConstructorReturn(this, babelHelpers.getPrototypeOf(Outer).call(this));

var _babelHelpers$get$cal = babelHelpers.get(babelHelpers.getPrototypeOf(Outer.prototype), "toString", babelHelpers.assertThisInitialized(_this)).call(babelHelpers.assertThisInitialized(_this));
_babelHelpers$get$cal = babelHelpers.get(babelHelpers.getPrototypeOf(Outer.prototype), "toString", babelHelpers.assertThisInitialized(_this)).call(babelHelpers.assertThisInitialized(_this));

let Inner = function Inner() {
babelHelpers.classCallCheck(this, Inner);
Expand Down
@@ -1,24 +1,19 @@
var _one, _ref, _undefined, _computed, _computed2, _ref2, _ref3, _baz, _ref4;

var foo = "foo";

var bar = () => {};

var four = 4;

var _one = one();

var _ref = 2 * four + seven;

var _undefined = undefined;

var _computed = computed();

var _computed2 = computed();

var _ref2 = "test" + one;

var _ref3 = /regex/;
var _baz = baz;
var _ref4 = `template${expression}`;
_one = one();
_ref = 2 * four + seven;
_undefined = undefined;
_computed = computed();
_computed2 = computed();
_ref2 = "test" + one;
_ref3 = /regex/;
_baz = baz;
_ref4 = `template${expression}`;

var MyClass =
/*#__PURE__*/
Expand Down
@@ -1,5 +1,7 @@
function test(x) {
var _x = x;
var _x;

_x = x;

var F = function F() {
"use strict";
Expand Down
@@ -0,0 +1,5 @@
const createClass = (k) => class { [k()] = 2 };

const clazz = createClass(() => 'foo');
const instance = new clazz();
expect(instance.foo).toBe(2);
@@ -0,0 +1 @@
const createClass = (k) => class { [k()] = 2 };
@@ -0,0 +1,18 @@
var createClass = k => {
var _temp;

var _k;

return _temp = (_k = k(),
/*#__PURE__*/
function () {
"use strict";

function _class2() {
babelHelpers.classCallCheck(this, _class2);
babelHelpers.defineProperty(this, _k, 2);
}

return _class2;
}()), _temp;
};
@@ -1,24 +1,19 @@
var _one, _ref, _undefined, _computed, _computed2, _ref2, _ref3, _baz, _ref4;

var foo = "foo";

var bar = () => {};

var four = 4;

var _one = one();

var _ref = 2 * four + seven;

var _undefined = undefined;

var _computed = computed();

var _computed2 = computed();

var _ref2 = "test" + one;

var _ref3 = /regex/;
var _baz = baz;
var _ref4 = `template${expression}`;
_one = one();
_ref = 2 * four + seven;
_undefined = undefined;
_computed = computed();
_computed2 = computed();
_ref2 = "test" + one;
_ref3 = /regex/;
_baz = baz;
_ref4 = `template${expression}`;

var MyClass =
/*#__PURE__*/
Expand Down
@@ -1,5 +1,7 @@
function test(x) {
var _x = x;
var _x;

_x = x;

var F = function F() {
"use strict";
Expand Down
Expand Up @@ -75,9 +75,11 @@ new ComputedMethod(); // ensure ComputedKey Field is still transformed

class ComputedField extends Obj {
constructor() {
let _ref;

var _temp3;

var _ref = (_temp3 = super(), babelHelpers.defineProperty(this, "field", 1), _temp3);
_ref = (_temp3 = super(), babelHelpers.defineProperty(this, "field", 1), _temp3);

class B extends Obj {
constructor() {
Expand Down
@@ -0,0 +1,23 @@
const classes = [];
for (let i = 0; i <= 10; ++i) {
classes.push(
class A {
[i] = `computed field ${i}`;
static foo = `static field ${i}`;
#bar = `private field ${i}`;
getBar() {
return this.#bar;
}
}
);
}

for(let i=0; i<= 10; ++i) {
const clazz = classes[i];
expect(clazz.foo).toBe('static field ' + i);

const instance = new clazz();
expect(Object.getOwnPropertyNames(instance)).toEqual([String(i)])
expect(instance[i]).toBe('computed field ' + i);
expect(instance.getBar()).toBe('private field ' + i);
}
@@ -0,0 +1,13 @@
const classes = [];
for (let i = 0; i <= 10; ++i) {
classes.push(
class A {
[i] = `computed field ${i}`;
static foo = `static field ${i}`;
#bar = `private field ${i}`;
getBar() {
return this.#bar;
}
}
);
}
@@ -0,0 +1,3 @@
{
"plugins": ["external-helpers", "proposal-class-properties"]
}
@@ -0,0 +1,23 @@
const classes = [];

for (let i = 0; i <= 10; ++i) {
var _class, _temp, _bar;

let _i;

classes.push((_temp = (_i = i, _class = class A {
constructor() {
babelHelpers.defineProperty(this, _i, `computed field ${i}`);

_bar.set(this, {
writable: true,
value: `private field ${i}`
});
}

getBar() {
return babelHelpers.classPrivateFieldGet(this, _bar);
}

}), _bar = new WeakMap(), babelHelpers.defineProperty(_class, "foo", `static field ${i}`), _temp));
}
@@ -1,4 +1,6 @@
var _class, _descriptor, _Symbol$search, _temp;
let _Symbol$search;

var _class, _descriptor, _temp;

function _initializerDefineProperty(target, property, descriptor, context) { if (!descriptor) return; Object.defineProperty(target, property, { enumerable: descriptor.enumerable, configurable: descriptor.configurable, writable: descriptor.writable, value: descriptor.initializer ? descriptor.initializer.call(context) : void 0 }); }

Expand Down
@@ -1,10 +1,12 @@
let _x$x;

function _classCallCheck(instance, Constructor) { if (!(instance instanceof Constructor)) { throw new TypeError("Cannot call a class as a function"); } }

function _defineProperty(obj, key, value) { if (key in obj) { Object.defineProperty(obj, key, { value: value, enumerable: true, configurable: true, writable: true }); } else { obj[key] = value; } return obj; }

function _classNameTDZError(name) { throw new Error("Class \"" + name + "\" cannot be referenced in computed property keys."); }

var _x$x = {
_x$x = {
x: (_classNameTDZError("A"), A) || 0
}.x;

Expand Down
@@ -1,10 +1,12 @@
let _ref;

function _classCallCheck(instance, Constructor) { if (!(instance instanceof Constructor)) { throw new TypeError("Cannot call a class as a function"); } }

function _defineProperty(obj, key, value) { if (key in obj) { Object.defineProperty(obj, key, { value: value, enumerable: true, configurable: true, writable: true }); } else { obj[key] = value; } return obj; }

function _classNameTDZError(name) { throw new Error("Class \"" + name + "\" cannot be referenced in computed property keys."); }

var _ref = (_classNameTDZError("C"), C) + 3;
_ref = (_classNameTDZError("C"), C) + 3;

let C = function C() {
"use strict";
Expand Down
@@ -1,6 +1,8 @@
let _ref;

function _classNameTDZError(name) { throw new Error("Class \"" + name + "\" cannot be referenced in computed property keys."); }

var _ref = (_classNameTDZError("C"), C) + 3;
_ref = (_classNameTDZError("C"), C) + 3;

class C {}

Expand Down
6 changes: 2 additions & 4 deletions packages/babel-traverse/src/path/modification.js
Expand Up @@ -24,9 +24,7 @@ export function insertBefore(nodes) {
) {
return parentPath.insertBefore(nodes);
} else if (
(this.isNodeType("Expression") &&
this.listKey !== "params" &&
this.listKey !== "arguments") ||
(this.isNodeType("Expression") && !this.isJSXElement()) ||
(parentPath.isForStatement() && this.key === "init")
) {
if (this.node) nodes.push(this.node);
Expand Down Expand Up @@ -220,7 +218,7 @@ export function unshiftContainer(listKey, nodes) {
key: 0,
});

return path.insertBefore(nodes);
return path._containerInsertBefore(nodes);
}

export function pushContainer(listKey, nodes) {
Expand Down
39 changes: 39 additions & 0 deletions packages/babel-traverse/test/modification.js
Expand Up @@ -36,6 +36,19 @@ describe("modification", function() {

expect(generateCode(rootPath)).toBe("function test(a) {\n b;\n}");
});

it("properly handles more than one arguments", function() {
const code = "foo(a, b);";
const ast = parse(code);
traverse(ast, {
CallExpression: function(path) {
path.pushContainer("arguments", t.identifier("d"));
expect(generateCode(path)).toBe("foo(a, b, d);");
path.pushContainer("arguments", t.stringLiteral("s"));
expect(generateCode(path)).toBe(`foo(a, b, d, "s");`);
},
});
});
});
describe("unshiftContainer", function() {
it("unshifts identifier into params", function() {
Expand Down Expand Up @@ -116,6 +129,32 @@ describe("modification", function() {
);
});

it("returns inserted path with nested JSXElement", function() {
const ast = parse("<div><span>foo</span></div>", {
plugins: ["jsx"],
});
let path;
traverse(ast, {
Program: function(_path) {
path = _path.get("body.0");
},
JSXElement: function(path) {
const tagName = path.node.openingElement.name.name;
if (tagName !== "span") return;
path.insertBefore(
t.JSXElement(
t.JSXOpeningElement(t.JSXIdentifier("div"), [], false),
t.JSXClosingElement(t.JSXIdentifier("div")),
[],
),
);
},
});
expect(generateCode(path)).toBe(
"<div><div></div><span>foo</span></div>;",
);
});

describe("when the parent is an export declaration inserts the node before", function() {
it("the ExportNamedDeclaration", function() {
const bodyPath = getPath("export function a() {}", {
Expand Down