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.
+780 −5
Diff settings

Always

Just for now

@@ -223,13 +223,12 @@ export default class File extends Store {
return id;
}
addHelper(name: string): Object {
addHelper(name: string, deps: Array<string>): Object {
const declar = this.declarations[name];
if (declar) return declar;
if (!this.usedHelpers[name]) {
this.metadata.usedHelpers.push(name);
this.usedHelpers[name] = true;
}
const generator = this.get("helperGenerator");
@@ -241,7 +240,18 @@ export default class File extends Store {
return t.memberExpression(runtime, t.identifier(name));
}
const ref = getHelper(name);
const opts = {};
if (deps) {
for (const dep in deps) {
if (!this.usedHelpers[name]) {
throw "Helper dependencies must be added to the file first";

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Aug 16, 2017

Member

Can you wrap this with new Error()? Otherwise babel throws something like Cannot add property _babel to "Helper dependencies must be added to the file first" instead of just Helper dependencies must be added to the file first. (Because of

)

@nicolo-ribaudo

nicolo-ribaudo Aug 16, 2017

Member

Can you wrap this with new Error()? Otherwise babel throws something like Cannot add property _babel to "Helper dependencies must be added to the file first" instead of just Helper dependencies must be added to the file first. (Because of

)

} else {
opts["babelHelpers." + dep] = this.usedHelpers[name];

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Aug 16, 2017

Member

I think babel-template only replaces identifiers, so babelHelpers.helperName in helpers won't be replaced. You might use something like babelHelpers$helperName, but then it would need to be replaced when generating external helpers.

@nicolo-ribaudo

nicolo-ribaudo Aug 16, 2017

Member

I think babel-template only replaces identifiers, so babelHelpers.helperName in helpers won't be replaced. You might use something like babelHelpers$helperName, but then it would need to be replaced when generating external helpers.

This comment has been minimized.

@peey

peey Aug 20, 2017

Contributor

Hey. I completely missed this, I guess since it was working on babel tests I assumed it would be generating the correct code. Apparently not. Anyways, I'll track the issues with this feature separately in #6058. Also I don't intend to implement too complex features for this - just enough to make it work, because #5706 already is aimed at providing a superset of the functionality

@peey

peey Aug 20, 2017

Contributor

Hey. I completely missed this, I guess since it was working on babel tests I assumed it would be generating the correct code. Apparently not. Anyways, I'll track the issues with this feature separately in #6058. Also I don't intend to implement too complex features for this - just enough to make it work, because #5706 already is aimed at providing a superset of the functionality

}
}
}
const ref = getHelper(name, opts);
const uid = (this.declarations[name] = this.scope.generateUidIdentifier(
name,
));
@@ -260,6 +270,8 @@ export default class File extends Store {
});
}
this.usedHelpers[name] = uid.name;
return uid;
}
@@ -270,6 +270,151 @@ helpers.createClass = template(`
})()
`);
//NOTE: convention is 'descriptor' is used for element descriptors, while 'propertyDescriptor' is used for
//property descriptor in all helpers related to decorators-2
helpers.decorate = template(`
(function (constructor, undecorated, memberDecorators, heritage) {
const prototype = constructor.prototype;
let finishers = [];
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.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.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(
"property",
key,
isStatic,
propertyDescriptor,
)
const decorated = babelHelpers.decorateElement(elementDescriptor, decorators);
elementDescriptors.set([key, isStatic], decorated.descriptor);
for (const extra of decorated.extras) {
// extras is an array of element descriptors
elementDescriptors.set([extra.key, extra.isStatic], extra);
}
finishers = finishers.concat(decorated.finishers);
}
return function(classDecorators) {
const result = babelHelpers.decorateClass(
constructor,
classDecorators,
heritage,
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.

for (const elementDescriptor of result.elements) {
const target = elementDescriptor.isStatic ? constructor : prototype;
Object.defineProperty(
target,
elementDescriptor.key,
elementDescriptor.descriptor
);
}
for (let finisher of finishers) {
finisher.call(undefined, result.constructor);
}
return result.constructor;
};
});
`);
helpers.decorateElement = template(`
(function (descriptor, decorators) {
//spec uses the param "element" instead of "descriptor" and finds descriptor from it
let extras = [];
let finishers = [];
let previousDescriptor = descriptor;
for (let i = decorators.length - 1; i >= 0; i--) {
const decorator = decorators[i];
const result = decorator(previousDescriptor.descriptor);
//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
const extrasObject = result.extras;
if (extrasObject) {
for (const extra of extrasObject) {
extras.push(extra);
}
}
}
extras = babelHelpers.mergeDuplicateElements(extras);
return { descriptor: previousDescriptor, extras, finishers };
});
`);
helpers.decorateClass = template(`
(function (constructor, decorators, heritage, elementDescriptors) {
let elements = elementDescriptors;
let finishers = [];
let previousConstructor = constructor;
for (let i = decorators.length - 1; i >= 0; i--) {
const decorator = decorators[i];
const result = decorator(
previousConstructor,
heritage,
elements
);
previousConstructor = result.constructor;
if (result.finishers) {
// result.finishers is called 'finisher' in the spec
finishers = finishers.concat(result.finishers);
}
if (result.elements) {
elements = elements.concat(result.elements); //FIXME: for some reason using for of exhausts heap here
}
elements = babelHelpers.mergeDuplicateElements(elements);
}
return { constructor: previousConstructor, elements, finishers };
});
`);
helpers.defineEnumerableProperties = template(`
(function (obj, descs) {
for (var key in descs) {
@@ -417,6 +562,30 @@ 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) {
let elementMap = {};
let staticElementMap = {};
for (let elementDescriptor of elements) {
if (elementDescriptor.isStatic) {
staticElementMap[elementDescriptor.key] = elementDescriptor;
} else {
elementMap[elementDescriptor.key] = elementDescriptor;
}
}
return Object.values(elementMap).concat(Object.values(staticElementMap));
});
`);
helpers.newArrowCheck = template(`
(function (innerThis, boundThis) {
if (innerThis !== boundThis) {
@@ -1,10 +1,10 @@
import helpers from "./helpers";
export function get(name) {
export function get(name, opts) {
const fn = helpers[name];
if (!fn) throw new ReferenceError(`Unknown helper ${name}`);
return fn().expression;
return fn(opts).expression;
}
export const list = Object.keys(helpers)
@@ -0,0 +1,3 @@
src
test
*.log
@@ -0,0 +1,35 @@
# babel-plugin-syntax-decorators-2
> Updated parsing of decorators.
## Installation
```sh
npm install --save-dev babel-plugin-syntax-decorators-2
```
## Usage
### Via `.babelrc` (Recommended)
**.babelrc**
```json
{
"plugins": ["syntax-decorators-2"]
}
```
### Via CLI
```sh
babel --plugins syntax-decorators-2 script.js
```
### Via Node API
```javascript
require("babel-core").transform("code", {
plugins: ["syntax-decorators-2"]
});
```
@@ -0,0 +1,13 @@
{
"name": "babel-plugin-syntax-decorators-2",
"version": "7.0.0-alpha.15",
"description": "Updated parsing of decorators",
"repository": "https://github.com/babel/babel/tree/master/packages/babel-plugin-syntax-decorators-2",
"license": "MIT",
"main": "lib/index.js",
"keywords": [
"babel-plugin"
],
"dependencies": {},
"devDependencies": {}
}
@@ -0,0 +1,7 @@
export default function() {
return {
manipulateOptions(opts, parserOpts) {
parserOpts.plugins.push("decorators2");
},
};
}
@@ -0,0 +1,20 @@
{
"name": "babel-plugin-transform-decorators-2",
"version": "7.0.0-alpha.15",
"author": "Peeyush Kushwaha <peeyush.p97@gmail.com>",
"license": "MIT",
"description": "Transformer for stage-2 decorators",
"repository": "https://github.com/babel/babel/tree/master/packages/babel-plugin-transform-decorators-2",
"main": "lib/index.js",
"keywords": [
"babel",
"babel-plugin",
"decorators"
],
"dependencies": {
"babel-plugin-syntax-decorators-2": "7.0.0-alpha.15"
},
"devDependencies": {
"babel-helper-plugin-test-runner": "7.0.0-alpha.15"
}
}
Oops, something went wrong.
ProTip! Use n and p to navigate between commits in a pull request.