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.
+227 −158
Diff settings

Always

Just for now

Viewing a subset of changes. View all

Transform works great, move stuff to helpers

  • Loading branch information...
peey committed Jul 31, 2017
commit edb7ff208ee7b83df3288cf921dac27d2694aaac
@@ -270,6 +270,141 @@ helpers.createClass = template(`
})()
`);
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
for (const [key, isStatic] of undecorated) {
const target = isStatic ? constructor : prototype;
const propertyDescriptor = Object.getOwnPropertyDescriptor(target, key);
elementDescriptors[key] = makeElementDescriptor(
"property",
key,
isStatic,
propertyDescriptor,
);
}
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] || Object.getOwnPropertyDescriptor(target, key);
const elementDescriptor = makeElementDescriptor(
"property",
key,
isStatic,
propertyDescriptor,
);
const decorated = decorateElement(elementDescriptor, decorators);
elementDescriptors[key] = decorated.descriptor;
for (const extra of decorated.extras) {
// extras is an array of element descriptors
elementDescriptors[extra.key] = extra;
}
finishers = finishers.concat(decorated.finishers);
}
return function(classDecorators) {
const result = decorateClass(
constructor,
classDecorators,
heritage,
Object.values(elementDescriptors),
);
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.

for (const elementDescriptor of result.elements) {
const target = elementDescriptor.isStatic ? constructor : prototype;
Object.defineOwnProperty(
target,
elementDescriptor.key,
elementDescriptor.descriptor,
);
}
return result.constructor;
};
});
`);
helpers.decorateElement = template(`
(function (descriptor, decorators) {
//spec uses the param "element" instead of "descriptor" and finds descriptor from it
let extras = [];
const finishers = [];
let previousDescriptor = descriptor;
for (let i = decorators.length - 1; i >= 0; i--) {
const decorator = decorators[i];
const result = decorator(previousDescriptor);
const currentDescriptor = result.descriptor;
if (result.finisher) {
finishers.push(current.finisher);
result.finisher = undefined;
}
previousDescriptor = currentDescriptor;
const extrasObject = result.extras;
if (extrasObject) {
for (const extra of extrasObject) {
extras.push(extra);
}
}
}
extras = mergeDuplicateElements(extras);
return { descriptor: previousDescriptor, extras, finishers };
});
`);
helpers.decorateClass = template(`
(function (constructor, decorators, heritage, elementDescriptors) {
let elements = [];
let finishers = [];
let previousConstructor = constructor;
const previousDescriptors = elementDescriptors;
for (let i = decorators.length - 1; i >= 0; i--) {
const decorator = decorators[i];
const result = decorator(
previousConstructor,
heritage,
previousDescriptors,
);
previousConstructor = result.constructor;
if (result.finishers) {
// result.finishers is called 'finisher' in the spec
finishers = finishers.concat(result.finishers);
}
if (result.elements) {
for (const element of result.elements) {
elements.push(element);
}
}
elements = mergeDuplicateElements(elements);
}
return { constructor: previousConstructor, elements, finishers };
});
`);
helpers.defineEnumerableProperties = template(`
(function (obj, descs) {
for (var key in descs) {
@@ -417,6 +552,19 @@ helpers.interopRequireWildcard = template(`
})
`);
helpers.makeElementDescriptor = template(`
(function (kind, key, isStatic, descriptor, finisher) {
return { kind, key, isStatic, descriptor, finisher };

This comment has been minimized.

@littledan

littledan Aug 30, 2017

Minor, but the finisher property is only created if it is not undefined.

@littledan

littledan Aug 30, 2017

Minor, but the finisher property is only created if it is not undefined.

});
`);
//TODO

This comment has been minimized.

@littledan

littledan Aug 30, 2017

Would be nice to include in the comment here that the details of merging are still up for discussion in TC39, per a note in the spec.

@littledan

littledan Aug 30, 2017

Would be nice to include in the comment here that the details of merging are still up for discussion in TC39, per a note in the spec.

This comment has been minimized.

@littledan

littledan Aug 30, 2017

The merging here seems like just overwriting and choosing the last one. I think merging should also include merging getter/setter pairs (that's the entire point).

@littledan

littledan Aug 30, 2017

The merging here seems like just overwriting and choosing the last one. I think merging should also include merging getter/setter pairs (that's the entire point).

This comment has been minimized.

@peey

peey Sep 16, 2017

Contributor

@littledan, I have a question. Does merging also mean that if by a previous decorator you have some attribute, then if it isn't provided by a later decorator with an extra for the same key, then we just use the attribute from the previous extra object? For example, if enumerability is set to false for an extra with key "foo" by a previous decorator, then a later decorator needn't provide a enumerability value for an extra with key "foo", and we'll just use the previous enumerability value?

Current behaviour in this PR expects that all extras are well-formed descriptors, which means that we simply overwrite and choose the last one, as you noticed.

@peey

peey Sep 16, 2017

Contributor

@littledan, I have a question. Does merging also mean that if by a previous decorator you have some attribute, then if it isn't provided by a later decorator with an extra for the same key, then we just use the attribute from the previous extra object? For example, if enumerability is set to false for an extra with key "foo" by a previous decorator, then a later decorator needn't provide a enumerability value for an extra with key "foo", and we'll just use the previous enumerability value?

Current behaviour in this PR expects that all extras are well-formed descriptors, which means that we simply overwrite and choose the last one, as you noticed.

helpers.mergeDuplicateElements = template(`
(function (elements) {
return elements;
});
`);
helpers.newArrowCheck = template(`
(function (innerThis, boundThis) {
if (innerThis !== boundThis) {
Oops, something went wrong.
ProTip! Use n and p to navigate between commits in a pull request.