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

Add support for the new decorators proposal #7976

Merged
merged 2 commits into from Sep 7, 2018

Conversation

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented May 18, 2018

Repl: https://babeljs.io/repl/build/8579/ (need to turn on the stage-2 preset)

Repo: https://github.com/tc39/proposal-decorators

Q                       A
Fixed Issues? Closes #7542, closes #6107, fixes #7773
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature? 🎉
Tests Added + Pass? Yes
Documentation PR
Any Dependency Changes?
License MIT

Support for private elements is still missing, but I will work on it after that this PR and #7842 are merged.

This PR should be merged after #7938 and #7948, since I will add some tests for them in this PR.

cc @littledan


Hi possible reviewers 🙂
Since this PR is quite big, it would be very helpful if you could review even just a part of it:


TODO:

  • Revert [REVERT BEFORE MERGING] commits
@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented May 18, 2018

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/9044/

@nicolo-ribaudo nicolo-ribaudo referenced this pull request May 18, 2018

Merged

Update syntax-decorators options #7938

1 of 1 task complete
"@babel/helper-plugin-test-runner": "7.0.0-beta.47"
"@babel/helper-plugin-test-runner": "7.0.0-beta.47",
"@babel/helper-replace-supers": "7.0.0-beta.47",
"@babel/helper-split-export-declaration": "7.0.0-beta.47"

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo May 22, 2018

Author Member

Reminder to myself - this should be a dependency.

@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:decorators-transform branch from f6239a6 to 2026871 May 30, 2018

@@ -37,7 +37,7 @@ export default declare((api, opts = {}) => {
return {
presets: [[presetStage3, { loose, useBuiltIns }]],
plugins: [
[transformDecorators, { legacy: decoratorsLegacy }],
[transformDecorators, { legacy: decoratorsLegacy && false }],

This comment has been minimized.

@jsg2021

jsg2021 Jun 1, 2018

this will prevent decoratorsLegacy from ever affecting the value. It will always be false?

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Jun 1, 2018

Author Member

Nope, it's just to test decorators in the REPL before that this PR is meged. That is why I labeled the commit as [REVERT BEFORE MERGING] 😛

This comment has been minimized.

@jsg2021

jsg2021 Jun 1, 2018

oh, Ha... I didn't see the commit message :) sorry.

@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:decorators-transform branch from ba46a74 to 8b45686 Jun 2, 2018

@babel-bot babel-bot referenced this pull request Jun 3, 2018

Open

Decorators (Stage 2) #11

1 of 2 tasks complete

This was referenced Jun 3, 2018

@@ -28,6 +21,8 @@ export default declare((api, options) => {
if (typeof decoratorsBeforeExport !== "boolean") {
throw new Error("'decoratorsBeforeExport' must be a boolean.");
}
} else if (!legacy) {

This comment has been minimized.

@diervo

diervo Jun 3, 2018

Contributor

I don't think we need to the option to toggle between before and after exports no more.
I believe there were very strong blocking arguments towards the exports before.

@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:decorators-transform branch from 9e25568 to 6dd4a0f Jun 4, 2018

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

nicolo-ribaudo commented Jun 6, 2018

I added some commits to make the helpers match 1:1 the spec text

@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:decorators-transform branch 2 times, most recently from f30514b to ce8a181 Jul 3, 2018

@littledan
Copy link

littledan left a comment

This PR looks great. I don't see any non-trivial errors.

I haven't developed any sort of proper test plan, but you may want to add tests for finisher ordering and enumeration ordering of elements/extras.

}

expect(el).toEqual({
[Symbol.toStringTag]: "Field Descriptor",

This comment has been minimized.

@littledan

littledan Jul 5, 2018

This has changed--it's now "Descriptor". tc39/proposal-decorators#96 (ditto for Method Descriptor, etc)

@@ -1011,3 +1011,498 @@ helpers.classPrivateFieldSet = () => template.program.ast`
return value;
}
`;

// Don't review me, review babel-helpers/src/helpers/decorators.js :)

This comment has been minimized.

@littledan
// This is exposed to the user code
type ElementObjectInput = ElementDescriptor & {
[@@toStringTag]?: "Method Descriptor" | "Field Descriptor"

This comment has been minimized.

@littledan

littledan Jul 5, 2018

This has changed to just "Descriptor". Same for class descriptors. The change comes up in a few cases below.

*/

/*::
// Various combinations with/without extras and with one or manu finishers

This comment has been minimized.

@littledan

littledan Jul 5, 2018

Nit: s/manu/many/

finishers /*: ClassFinisher[] */,
) /*: Class<*> */ {
for (var i = 0; i < finishers.length; i++) {
var newConstructor /*: ?Class<*> */ = (0, finishers[i])(constructor);

This comment has been minimized.

@littledan

littledan Jul 5, 2018

I don't understand the purpose of 0, here--is this to avoid function name inference? I don't understand why that would occur. Ditto for a few other similar locations.

This comment has been minimized.

@Jessidhia

Jessidhia Jul 6, 2018

Member

This erases the this by separating the [[Get]] from [[Call]]. If finishers[i](constructor) was directly invoked, the finisher would receive the finishers list as the this.

This comment has been minimized.

@littledan

littledan Jul 6, 2018

Ah, of course. Thanks for explaining.

if (elementsAndFinisher.elements !== undefined) {
elements = elementsAndFinisher.elements;

for (var j = 0; j < elements.length - 1; j++) {

This comment has been minimized.

@littledan

littledan Jul 5, 2018

Maybe a good idea to insert a TODO to check if there's a problem caused by this quadratic algorithm, though it seems fine to start.

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Jul 10, 2018

Author Member

How else could it be implemented? Creating a Set instead of the array wouldn't work because we need to check some properties on the obejcts, not the objects themselves.

This comment has been minimized.

@littledan

littledan Jul 11, 2018

There are only three possible placements, so I think you can implement this as three Sets of keys. (This could be organized as, an object which has a property for each placement, which is a Set.)

for (var j = 0; j < newExtras.length; j++) {
_addElementPlacement(newExtras[j], placements);
}
extras = extras.concat(newExtras);

This comment has been minimized.

@littledan

littledan Jul 5, 2018

Consider including a TODO to fix this quadratic algorithm (extras will repeatedly be copied).


function _disallowProperty(obj, name) {
if (obj[name] !== undefined) {
throw new TypeError("Unexpected '" + name + "' property.");

This comment has been minimized.

@littledan

littledan Jul 5, 2018

A detailed error message here would be great (may mean that the function takes some extra arguments).

[push(13)]() {}
}

var numsFrom0to9 = Array.from({ length: 24 }, (_, i) => i);

This comment has been minimized.

@littledan

littledan Jul 5, 2018

Variable name?


function push(x) { log.push(x); return x; }

function logFinisher(a, b) {

This comment has been minimized.

@littledan

littledan Jul 5, 2018

This has to do with the execution of the decorator, not the finisher; consider renaming this test and adding other tests to check the ordering of finishers.

This comment has been minimized.

@xtuc xtuc closed this Jul 6, 2018

@xtuc xtuc reopened this Jul 6, 2018

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

nicolo-ribaudo commented Jul 8, 2018

Thanks for the review Daniel, I will update the PR in the next few days.

);
}

const { decoratorsBeforeExport } = options;

This comment has been minimized.

@hzoo

hzoo Jul 24, 2018

Member

nit: do we want to just move this destructuring to line 9?

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Aug 17, 2018

Author Member

I prefer to keep it there alongside with the decoratorsBeforeExport validation.

@@ -0,0 +1,4 @@
{
"plugins": ["proposal-decorators", "proposal-class-properties", "external-helpers"],
"presets": ["env"]

This comment has been minimized.

@hzoo

hzoo Jul 24, 2018

Member

I think it's ok to not use env in these tests? What do you think?

@hzoo

hzoo approved these changes Jul 24, 2018

Copy link
Member

hzoo left a comment

Awesome work! I think we can clean up the test a bit by not running the test of the env preset tho

Curious about sharing the helper functions in src/transformer.js like prop, value since it's probably in @babel/types but really not a big deal, can refactor that stuff later.

@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:decorators-transform branch from b66722a to ba7d327 Aug 17, 2018

@littledan
Copy link

littledan left a comment

Thanks for fixing the initializer issue. I don't have any further concerns.

@littledan

This comment has been minimized.

Copy link

littledan commented Sep 5, 2018

What is blocking this PR from landing?

@jsg2021

This comment has been minimized.

Copy link

jsg2021 commented Sep 5, 2018

export placement? stage advancement?

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

nicolo-ribaudo commented Sep 6, 2018

Actually nothing, but since this PR is quite big I'm waiting for more reviews.

@pabloalmunia

This comment has been minimized.

Copy link

pabloalmunia commented Sep 6, 2018

I'm read the new code and look very well. Please, go a head with this important contribution ASAP.

@nicolo-ribaudo very good work, congratulations.

@existentialism
Copy link
Member

existentialism left a comment

👏 les do it!

@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:decorators-transform branch from ba7d327 to b304993 Sep 6, 2018

@loganfsmyth
Copy link
Member

loganfsmyth left a comment

Awesome work. I'm leaving some comments, but I don't think any of them are realistically blockers, so if you want to land this and address them in a second PR, that's 100% fine.

if (!legacy) {
throw new Error(
"The decorators plugin requires a 'decoratorsBeforeExport' option," +
" whose value must be a boolean.",

This comment has been minimized.

@loganfsmyth

loganfsmyth Sep 6, 2018

Member

Can we mention legacy: true in here too so it is more discoverable?

return false;
}

function extractDecorators({ node }) {

This comment has been minimized.

@loganfsmyth

loganfsmyth Sep 6, 2018

Member

nitpick: From a naming standpoint, takeDecorators might make it clearer that this also clears the decorators.

if (node.computed) {
return node.key;
} else {
return t.stringLiteral(node.key.name || String(node.key.value));

This comment has been minimized.

@loganfsmyth

loganfsmyth Sep 6, 2018

Member

I think explicitness here can be a good thing, e.g.

if (node.computed) {
  return node.key;
} else if (t.isIdentifier(node.key)) {
  return t.stringLiteral(node.key.name);
} else {
  return t.stringLiteral(`${node.key.value}`);
}
}

function getElementsDefinitions(path, fId, file) {
const superRef = path.node.superClass || t.identifier("Function");

This comment has been minimized.

@loganfsmyth

loganfsmyth Sep 6, 2018

Member

Is Function here right? It looks like ReplaceSupers already defaults to Function.prototype when there is no super class. Is that not right?

}

function value(body, params = []) {
return t.objectMethod("method", t.identifier("value"), params, body);

This comment has been minimized.

@loganfsmyth

loganfsmyth Sep 6, 2018

Member

How do you feel about generating an object method vs a property containing a function expression here? I usually prefer to generate ES5 code, but what you have is also fine.

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Sep 7, 2018

Author Member

I think that ES5 was a good target for ES2015/ES2016 transforms, but now we can output ES6 since users will either only support browsers which support ES6 or compile it down with preset-env.

This comment has been minimized.

@Jessidhia

Jessidhia Sep 7, 2018

Member

We still have to be careful with helpers, they still have to be as ES3-compatible as possible; but generated output should be safe to use the latest ES20xx syntax that was current before it is made stage-4 I think.

F: A,
d: []
};
}, (await B));

This comment has been minimized.

@loganfsmyth

loganfsmyth Sep 6, 2018

Member

That's a fun edge case. Do we need to worry about

@dec
class Foo {
  [await prop()](){}
}

and

class Foo {
  @(await thing())
  method(){}
}

too?

Might at least be good to explicitly throw an error about that being unsupported. Same for yield.

This comment has been minimized.

@Jessidhia

Jessidhia Sep 7, 2018

Member

Those look like perfectly valid syntax to me (inside an async function) 🤔

Or does being inside a class override the +async?

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Sep 7, 2018

Author Member

It's tracked at #8300. I will add a nice error message.

@Kovensky It's just very hard to support (we don't support it in computer keys either), I will revisit it in another PR.

This comment has been minimized.

@Jessidhia

Jessidhia Sep 7, 2018

Member

Ah, a problem with our generated function wrapper...

superClass /*: ?Class<*> */,
) /*: Class<*> */ {
var r = factory(function initialize(O) {
_initializeInstanceElements(O, decorated.elements);

This comment has been minimized.

@loganfsmyth

loganfsmyth Sep 6, 2018

Member

I'm trying to think, is it possible for this to get called before decorated is initialized? Something would have to call new on the class, which may not be possible?

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Sep 7, 2018

Author Member

Yeah, it isn't possible because the class can't be new-ed before being returned by the factory function.

// ClassDefinitionEvaluation (Steps 26-*)
export default function _decorate(
decorators /*: ClassDecorator[] */,

This comment has been minimized.

@loganfsmyth

loganfsmyth Sep 6, 2018

Member

Loving the type annotations by the way, thanks for doing that :D

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Sep 7, 2018

Author Member

With #8487 we could probably even type-check them 😛

insertInitializeInstanceElements(path, initializeId);

const expr = template.expression.ast`
${file.addHelper("decorate")}(

This comment has been minimized.

@loganfsmyth

loganfsmyth Sep 6, 2018

Member

It might be nice for users to wrap a try/catch here to give users more feedback, e.g.

function addDecorateHelper(file) {
  try {
    return file.addHelper("decorate");
  } catch (err) {
    if (err.code === "BABEL_HELPER_UNKNOWN") {
      err.message += "\n  @babel/plugin-transform-decorators in non-legacy mode requires @babel/core version ^7.0.1 and you appear to be using an older version";
    }
    throw err;
  }
}
${file.addHelper("decorate")}(
${classDecorators || t.nullLiteral()},
function (${initializeId}, ${superClass ? superId : null}) {
${isStrict ? null : t.stringLiteral("use strict")}

This comment has been minimized.

@loganfsmyth

loganfsmyth Sep 6, 2018

Member

This isn't quite the right structure unfortunately, because use strict should be a t.directive, and also because this will insert it into body instead of the directives array. I don't think there's an easy way to create an optional directive with babel-templates at the moment unfortunately. I think we'd have to inject it manually after the template AST is created.

@nicolo-ribaudo nicolo-ribaudo merged commit 9aec4ad into babel:master Sep 7, 2018

4 checks passed

babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 80.62% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nicolo-ribaudo nicolo-ribaudo deleted the nicolo-ribaudo:decorators-transform branch Sep 7, 2018

@pabloalmunia

This comment has been minimized.

Copy link

pabloalmunia commented Sep 18, 2018

This message is displayed when 'decoratorsBeforeExport' isn't include:

"The decorators plugin requires a 'decoratorsBeforeExport' option, whose value must be a boolean. If you want to use the legacy decorators semantics, you can set the 'legacy: true' option."

But the documentation say 'decoratorsBeforeExport: boolean, defaults to false.'

@jsg2021

This comment has been minimized.

Copy link

jsg2021 commented Sep 18, 2018

I think the docs are wrong. You have to choose a value. The proposed spec hasn’t settled the placement yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.