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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions packages/babel-helper-create-class-features-plugin/src/misc.js
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
function test(x) {
var _x = x;
var _x;

_x = x;

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

const clazz = createClass(() => 'foo');
const instance = new clazz();
expect(instance.foo).toBe(2);
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
const createClass = (k) => class { [k()] = 2 };
Original file line number Diff line number Diff line change
@@ -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;
};
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
function test(x) {
var _x = x;
var _x;

_x = x;

var F = function F() {
"use strict";
Expand Down
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
@@ -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);
}
Original file line number Diff line number Diff line change
@@ -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;
}
}
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"plugins": ["external-helpers", "proposal-class-properties"]
}
Original file line number Diff line number Diff line change
@@ -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));
}
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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