-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Changes from 14 commits
57cdfaf
9904b61
1d64084
f214d1d
edb7ff2
10d763e
358cf70
626b92e
d8cebba
ef4eba5
b1092ca
a55e374
5f7bf93
8707f4e
6ec36a1
2e7c6a1
8e28ca7
1776e24
4e527a3
44465f2
f86b327
96f51aa
76f5d3c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"; | ||
} else { | ||
opts["babelHelpers." + dep] = this.usedHelpers[name]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we ES2015 here? And a bit later, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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"? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "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 }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor, but the finisher property is only created if it is not undefined. |
||
}); | ||
`); | ||
|
||
//TODO | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 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) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
src | ||
test | ||
*.log |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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"] | ||
}); | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
{ | ||
"name": "babel-plugin-syntax-decorators-2", | ||
"version": "7.0.0-alpha.19", | ||
"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": {} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
export default function() { | ||
return { | ||
manipulateOptions(opts, parserOpts) { | ||
parserOpts.plugins.push("decorators2"); | ||
}, | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
{ | ||
"name": "babel-plugin-transform-decorators-2", | ||
"version": "7.0.0-alpha.19", | ||
"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.19" | ||
}, | ||
"devDependencies": { | ||
"babel-helper-plugin-test-runner": "7.0.0-alpha.19", | ||
"babel-helper-transform-fixture-test-runner": "7.0.0-alpha.19", | ||
"babel-plugin-transform-decorators-2": "7.0.0-alpha.19" | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you wrap this with
new Error()
? Otherwise babel throws something likeCannot add property _babel to "Helper dependencies must be added to the file first"
instead of justHelper dependencies must be added to the file first
. (Because ofbabel/packages/babel-core/src/transformation/file/index.js
Line 462 in 605adc9