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

Leave natural this in the decorator functions #487

Closed
pabloalmunia opened this issue Nov 8, 2022 · 7 comments
Closed

Leave natural this in the decorator functions #487

pabloalmunia opened this issue Nov 8, 2022 · 7 comments

Comments

@pabloalmunia
Copy link
Contributor

pabloalmunia commented Nov 8, 2022

Currently

In the specification, the decorator functions have a call that sets the value undefined to this.

 Call(decorator, undefined, « value, context »).

Curiously, in the current implementation of Babel this is true in all cases except for class decorators, in which this is the decorator function (the natural behavior):

function decorator(value, context) {
  console.log('decorator call');
  console.log('this', this);
  console.log('value', value);
  console.log('context', context);
}

@decorator
class C {
  @decorator
  m() {}
}

The output for the method decorator is:

decorator call
this undefined
value [Function: m]
context {
  kind: 'method',
  name: 'm',
  isStatic: false,
  isPrivate: false,
  addInitializer: [Function (anonymous)],
  getMetadata: [Function: getMetadata],
  setMetadata: [Function: setMetadata]
}

The output for the class decorator is:

decorator call
this [ [Function: decorator] ]
value [class C]
context {
  kind: 'class',
  name: 'C',
  addInitializer: [Function (anonymous)],
  getMetadata: [Function: getMetadata],
  setMetadata: [Function: setMetadata]
}

Why is this undefined?

In some discussions, it was suggested that this could be the decorated element, but this option brought a lot of problems and side effects. Probably, the solution was made to assign the value undefined to this to avoid these problems.

In my humble opinion, forcing the undefined value in this is an over-specification, and it would be correct to let the decorator function have the natural this that it has.

Advantages of leaving this of the decorating function in a natural way

A class could be considered a decorator factory, where when we create an instance, we generate a reference that can use for all calls to a method as a decorator.

An example of this behavior can be seen here: JavaScript metaprogramming with the 2022-03 decorators API (2ality.com)

As I understand it, if Babel followed the specification, this example would not be possible. Please correct me if I am wrong.

Proposal of update

Remove the assignment of this with the undefined value in the decorator function call. It's a minor change but essential.

@trusktr
Copy link
Contributor

trusktr commented Nov 12, 2022

That 2ality arrow-function example will always work regardless what the spec chooses to do, because the arrow function always captures (or "closes over") this from the scope where it is used.


Are you suggesting that @some.method should be treated similarly to some.method() and automatically use some as this when it calls method?

That would be interesting!

Whereas @(some.method) would be as if we wrote (some.method)() and would lose this because some.method is being used as an expression.

This would simply work:

class Foo {
  decorate() {
    console.log(this instanceof Foo) // true
  }
}

const foo = new Foo()

@foo.decorate
class A {}

// Outputted "true" to console.

That's nice!

It is not too late to change this part of the design because no JS runtime has implemented decorators yet.

@nicolo-ribaudo
Copy link
Member

Note that (some.method)() doesn't loose this.

@pabloalmunia
Copy link
Contributor Author

Allow me to summarize what I have understood up to this point.

Current specification

class Decorator {
  name = "Decorator factory";
  check(value, context) {
    console.log('decorator call');
    console.log('this', this);
    console.log('value', value);
    console.log('context', context);
  }
}

const decorator = new Decorator;

@decorator.check
class C {
  @decorator.check
  m() {}
}
  1. In this simple example, this must be undefined in the check method-decorator.

  2. The current implementation in Babel has a minor error and the class method-decorator don't have undefined in this, but is a mistake. Is it true @nicolo-ribaudo?

Workarround: arrow function

class Decorator {
  name = "Decorator factory";
  check = (value, context) => {
    console.log('decorator call');
    console.log('this', this);
    console.log('value', value);
    console.log('context', context);
  }
}

const decorator = new Decorator;

@decorator.check
class C {
  @decorator.check
  m() {}
}
  1. If we use a arrow function into a member of Decorator class the this is the object instance.

Workarround: the method return the decorator

class Decorator {
  name = "Decorator factory";
  check() {
    const _this = this;
    return function(value, context) {
      console.log('decorator call');
      console.log('this', this);
      console.log('_this', _this);
      console.log('value', value);
      console.log('context', context);
    }
  }
}

const decorator = new Decorator;

@(decorator.check)()
class C {
  @(decorator.check)()
  m() {}
}

4.- With a method that returns the decorator we can keep the original by closure, but the method call must be as a expression.

Question

@pzuraq, why is it not possible to keep the natural this in decorators and avoid these workarrounds?

@pzuraq
Copy link
Collaborator

pzuraq commented Nov 25, 2022

I think this may have been a mistake in the spec, I can't think of a reason that we would have wanted to change this. I'll update it.

@Jamesernator
Copy link

I think this may have been a mistake in the spec, I can't think of a reason that we would have wanted to change this. I'll update it.

Is this change still planned? I've been playing around with decorators in TS 5.0's support and I was wanting to have some decorators as methods of a class.

(As far as I can tell in the current spec it just calls the decorator unconditionally with this as undefined)

@pzuraq
Copy link
Collaborator

pzuraq commented Feb 27, 2023

This is still planned, yes. However as a normative change now it will need to go through committee first.

pzuraq added a commit to pzuraq/ecma262 that referenced this issue Apr 12, 2023
@pzuraq
Copy link
Collaborator

pzuraq commented Apr 12, 2023

This has been resolved, natural this is used for decorator functions when called.

@pzuraq pzuraq closed this as completed Apr 12, 2023
pzuraq added a commit to pzuraq/ecma262 that referenced this issue Feb 2, 2024
Updates

- Refactor to use ClassElementDefinition more extensively
- Separate parsing class elements and defining them so that we can
  control the order of definition and decorator application
- Extract and centralize class element decoration into a single place
- Fix completions in ClassDefinitionEvaluation (need to reset the env
  after each possible abrupt completion).
- Refactor adding private methods to instance to pull directly from
  `Class.[[Elements]]`, so we don't have multiple sources of truth for
  the elements that are defined on a class. `Class.[[Elements]]` is the
  canonical source of truth after ClassDefinitionEvaluation, and any
  operations afterward such as instantiation should base themselves on
  that.
- Refactor to use anonymous function syntax.

Non-normative fixes

Fixes based on feedback from @jmdyck

Remove dynamic [[HomeObject]] from decorator return values

Throw an error if the value passed to `addInitializer` is not callable

Set the name of the `addInitializer` function

Remove setFunctionName from decorator application

Call decorators with their natural `this` value.

For more details, see this issue: tc39/proposal-decorators#487

Reverse initializer order

Update field and accessor extra initializers to run after definition
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants