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

Decorators 2 Transform [WIP] #6107

Closed
wants to merge 23 commits into
base: master
from
Commits
Jump to file or symbol
Failed to load files and symbols.
+181 −17
Diff settings

Always

Just for now

Viewing a subset of changes. View all

Add examples, & tests for extras, finishers

  • Loading branch information...
peey committed Aug 15, 2017
commit a55e374de78681f87ef791dd6868e1c3fdd16475
@@ -276,24 +276,27 @@ helpers.decorate = template(`
(function (constructor, undecorated, memberDecorators, heritage) {
const prototype = constructor.prototype;
let finishers = [];
const elementDescriptors = {}; // elementDescriptors is meant to be an array, so this will be converted later
//TODO: merging of elementDescriptors
const elementDescriptors = new Map(); // elementDescriptors is meant to be an array, so this will be converted later
for (const [key, isStatic] of undecorated) {
const target = isStatic ? constructor : prototype;
const propertyDescriptor = Object.getOwnPropertyDescriptor(target, key);
elementDescriptors[key] = babelHelpers.makeElementDescriptor(
"property",
key,
isStatic,
propertyDescriptor,
elementDescriptors.set(
[key, isStatic],
babelHelpers.makeElementDescriptor(
"property",
key,
isStatic,
propertyDescriptor,
)
);
}
// decorate and store in elementDescriptors
for (const [key, decorators, isStatic] of memberDecorators) {

This comment has been minimized.

@xtuc

xtuc Aug 16, 2017

Member

Can we ES2015 here?

And a bit later, Array.from will require a polyfill.
It could cause issues like this #5876, right?

@xtuc

xtuc Aug 16, 2017

Member

Can we ES2015 here?

And a bit later, Array.from will require a polyfill.
It could cause issues like this #5876, right?

This comment has been minimized.

@peey

peey Aug 16, 2017

Contributor

I wish we could. But if we can't, I'll refactor it to use ES5, it shouldn't be complex. I'll leave this be till the end though, since the code is functionally correct so for the review we could focus on just that

@peey

peey Aug 16, 2017

Contributor

I wish we could. But if we can't, I'll refactor it to use ES5, it shouldn't be complex. I'll leave this be till the end though, since the code is functionally correct so for the review we could focus on just that

const target = isStatic ? constructor : prototype;
const propertyDescriptor =
elementDescriptors[key] && elementDescriptors[key].descriptor
elementDescriptors.has([key, isStatic]) && elementDescriptors.get([key, isStatic]).descriptor

This comment has been minimized.

@littledan

littledan Aug 30, 2017

This has call will always return false, since ES6 maps are by identity. Since [1, 2] !== [1, 2], calling Map.prototype.has on a fresh array will never do what you're hoping for.

Instead, you could maintain two Sets of keys, or two objects with keys and values, for the static and non-static halves.

@littledan

littledan Aug 30, 2017

This has call will always return false, since ES6 maps are by identity. Since [1, 2] !== [1, 2], calling Map.prototype.has on a fresh array will never do what you're hoping for.

Instead, you could maintain two Sets of keys, or two objects with keys and values, for the static and non-static halves.

|| Object.getOwnPropertyDescriptor(target, key);
const elementDescriptor = babelHelpers.makeElementDescriptor(
@@ -304,11 +307,11 @@ helpers.decorate = template(`
)
const decorated = babelHelpers.decorateElement(elementDescriptor, decorators);
elementDescriptors[key] = decorated.descriptor;
elementDescriptors.set([key, isStatic], decorated.descriptor);
for (const extra of decorated.extras) {
// extras is an array of element descriptors
elementDescriptors[extra.key] = extra;
elementDescriptors.set([extra.key, extra.isStatic], extra);
}
finishers = finishers.concat(decorated.finishers);
@@ -319,8 +322,9 @@ helpers.decorate = template(`
constructor,
classDecorators,
heritage,
Object.values(elementDescriptors)
Array.from(elementDescriptors.values())
);
finishers = finishers.concat(result.finishers);
//TODO: heritage hacks so result.constructor has the correct prototype and instanceof results

This comment has been minimized.

@littledan

littledan Aug 30, 2017

Heritage is just given as an argument to class decorators; it's the responsibility of the decorators to do the appropriate thing with that, I believe.

@littledan

littledan Aug 30, 2017

Heritage is just given as an argument to class decorators; it's the responsibility of the decorators to do the appropriate thing with that, I believe.

//TODO: step 38 and 39, what do they mean "initialize"?

This comment has been minimized.

@littledan

littledan Aug 30, 2017

"Initialize" was about having methods and the constructor start in some state where they would be unusable, and make them usable later. In my follow-on decorators proposal, I'm suggesting to get rid of this initialized/uninitialized state. I think it's OK to go without it to start, even if it's possible that TC39 will ask to bring it back.

@littledan

littledan Aug 30, 2017

"Initialize" was about having methods and the constructor start in some state where they would be unusable, and make them usable later. In my follow-on decorators proposal, I'm suggesting to get rid of this initialized/uninitialized state. I think it's OK to go without it to start, even if it's possible that TC39 will ask to bring it back.

@@ -334,6 +338,10 @@ helpers.decorate = template(`
);
}
for (let finisher of finishers) {
finisher.call(undefined, result.constructor);
}
return result.constructor;
};
});
@@ -343,17 +351,18 @@ helpers.decorateElement = template(`
(function (descriptor, decorators) {
//spec uses the param "element" instead of "descriptor" and finds descriptor from it
let extras = [];
const finishers = [];
let finishers = [];
let previousDescriptor = descriptor;
for (let i = decorators.length - 1; i >= 0; i--) {
const decorator = decorators[i];
const result = decorator(previousDescriptor.descriptor);
if (result.finisher) {
finishers.push(current.finisher);
result.finisher = undefined;
//TODO: why does .finisher exist on an elementDescriptor? the following conditional deviates from
//the spec because it uses result.finishers rather than result.descriptor.finisher
if (result.finishers) {
finishers = finishers.concat(result.finishers);
}
previousDescriptor.descriptor = result.descriptor; // just change the property descriptor
@@ -563,12 +572,17 @@ helpers.makeElementDescriptor = template(`
helpers.mergeDuplicateElements = template(`
(function (elements) {
let elementMap = {};
let staticElementMap = {};
for (let elementDescriptor of elements) {
elementMap[elementDescriptor.key] = elementDescriptor;
if (elementDescriptor.isStatic) {
staticElementMap[elementDescriptor.key] = elementDescriptor;
} else {
elementMap[elementDescriptor.key] = elementDescriptor;
}
}
return Object.values(elementMap);
return Object.values(elementMap).concat(Object.values(staticElementMap));
});
`);
@@ -0,0 +1,37 @@
function dec(descriptor) {
const extra = {
kind: "property",
key: "magic",
isStatic: false,
descriptor: {
enumerable: true,
configurable: true,
writable: true,
value: function () {return "extra"}
}
}
const extraStatic = {
kind: "property",
key: "magic",
isStatic: true,
descriptor: {
enumerable: true,
configurable: true,
writable: true,
value: function () {return "extraStatic"}
}
}
return {descriptor, extras: [extra, extraStatic], finishers: []}
}
class Foo {
@dec method() {return "method"}
}
var x = new Foo();
assert.equal(x.method(), "method");
assert.equal(x.magic(), "extra");
assert.equal(Foo.magic(), "extraStatic");
@@ -0,0 +1,35 @@
const logs = [];
function overrider(fn) {
return function(descriptor) {
descriptor.value = fn;
return {descriptor, extras: [], finishers: []};
}
}
function dec(log) {
return function (descriptor) {
const finisher = function (ctor) {
const x = new ctor();
assert(x.method1);
assert(x.method2);
assert.equal(x.method1(), "method1");
// making sure that ctor passed to the finisher has all the decorators applied
assert.equal(x.method2(), "method2 overridden");
logs.push("finisher called " + log);
}
return {descriptor, extras: [], finishers: [finisher]}
}
}
function classDec(constructor, heritage, elements) {
return {constructor, elements, finishers: [() => logs.push("finisher called classDec")]};
}
@classDec class Foo {
@dec(1) method1() {return "method1"}
@dec(2) @overrider(() => "method2 overridden") method2() {return "method2"};
}
assert.deepEqual(logs, ["finisher called 1", "finisher called 2", "finisher called classDec"]);
@@ -0,0 +1,31 @@
const calls = [];
function log(message) {
return function (descriptor) {
let oldFunc = descriptor.value;
descriptor.value = function(...args) {

This comment has been minimized.

@Kovensky

Kovensky Aug 16, 2017

Member

Somewhat unfortunately, there is no way of making this function's name derive from the original function name (instead of "value") other than by calling defineProperty on it afterwards 🤔

@Kovensky

Kovensky Aug 16, 2017

Member

Somewhat unfortunately, there is no way of making this function's name derive from the original function name (instead of "value") other than by calling defineProperty on it afterwards 🤔

This comment has been minimized.

@peey

peey Aug 16, 2017

Contributor

Actually, we'll probably be passing the element descriptor to the decorator and not the property descriptor. I just had a doubt on how that'd work when you change the key of the descriptor you've returned and that's why I made the call to implement it like this. Once I get a little clarification on assumption #4 from @littledan or someone, I'll make the desired changes

@peey

peey Aug 16, 2017

Contributor

Actually, we'll probably be passing the element descriptor to the decorator and not the property descriptor. I just had a doubt on how that'd work when you change the key of the descriptor you've returned and that's why I made the call to implement it like this. Once I get a little clarification on assumption #4 from @littledan or someone, I'll make the desired changes

calls.push(message + " entered");
let result = oldFunc.apply(this, args);
calls.push(message + " exited");
return result;
}
return {descriptor, extras: [], finishers: []};
}
}
class Foo {
@log("method1") method1() {
assert.equal(this.method2(), 6);
return 4;
}
@log("method2") method2() {return 6;}
@log("method3") method3() {return 8;}
}
let foo = new Foo();
assert.equal(foo.method1(), 4);
assert.equal(foo.method3(), 8);
assert.deepEqual(calls, ["method1 entered", "method2 entered", "method2 exited", "method1 exited", "method3 entered", "method3 exited"]);
@@ -0,0 +1,17 @@
function overrider(fn) {
return function(descriptor) {
descriptor.value = fn;
return {descriptor, extras: [], finishers: []};
}
}
class Foo {
@overrider(() => 3) method() {

This comment has been minimized.

@Kovensky

Kovensky Aug 16, 2017

Member

What is this here? Is it the constructor, the outer this, or the newly created instance?

Could be worth having a test for it.

@Kovensky

Kovensky Aug 16, 2017

Member

What is this here? Is it the constructor, the outer this, or the newly created instance?

Could be worth having a test for it.

return 2;
}
}
const x = new Foo();
assert.equal(x.method(), 3);
@@ -0,0 +1,30 @@
// makes an undecorated copy of the method
function spare(name, isStatic) {
return function(descriptor) {
const clone = {
kind: "property",
isStatic: !!isStatic,
key: name,

This comment has been minimized.

@Kovensky

Kovensky Aug 16, 2017

Member

What should happen if the name conflicts? @spare("foo") foo() {}

@Kovensky

Kovensky Aug 16, 2017

Member

What should happen if the name conflicts? @spare("foo") foo() {}

This comment has been minimized.

@peey

peey Aug 16, 2017

Contributor

Currently what I've interpreted from the spec is that it should "merge". By that I mean it will just take on the value of the last property added with the key "foo". See decorateElement step 6. Also see mergeDuplicateElements helper

@peey

peey Aug 16, 2017

Contributor

Currently what I've interpreted from the spec is that it should "merge". By that I mean it will just take on the value of the last property added with the key "foo". See decorateElement step 6. Also see mergeDuplicateElements helper

descriptor: Object.assign({}, descriptor)
}
return {descriptor, extras: [clone], finishers: []};
}
}
function overrider(fn) {
return function(descriptor) {
descriptor.value = fn;
return {descriptor, extras: [], finishers: []};
}
}
class Foo {
@overrider(() => 3) @spare("spared") method() {
return 2;
}
}
const x = new Foo();
assert.equal(x.spared(), 2);
assert.equal(x.method(), 3);
ProTip! Use n and p to navigate between commits in a pull request.