Reminder to myself - this should be a dependency.

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

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] 😛

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

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.

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

decorate.js?

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

Nit: s/manu/many/

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 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.

Ah, of course. Thanks for explaining.

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.

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.

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.)

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

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

Variable name?

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.

adding other tests to check the ordering of finishers

https://github.com/babel/babel/pull/7976/files/ce8a1818c9992377ed26767bd0336b20e6bd5318#diff-3a4ca738a2b3ce6ab485658c51556f68

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

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

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

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

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

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}`);
}

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

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.

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.

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.

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.

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

Or does being inside a class override the +async?

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.

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

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?

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

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

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

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;
  }
}

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.