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

Static decorators proposal iteration #250

Merged
merged 15 commits into from Mar 15, 2019
Merged

Static decorators proposal iteration #250

merged 15 commits into from Mar 15, 2019

Conversation

littledan
Copy link
Member

@littledan littledan commented Feb 25, 2019

@FireyFly
Copy link

Hm. I'm not a TC39 member, but.. I'm unsure what the benefit is from having decorators be a special kind of entity, rather than just a function taking and returning descriptor objects. This seems like a departure from other places where decorators are used (like property descriptors with Object.{defineProperty,getOwnPropertyDescriptor}).

I think this mostly makes the language a bit less orthogonal, which means more cases to take into account (when reading code, when writing specs, when implementing, etc). It also makes me wonder in what contexts I can use an @foo identifier (can I make an @foo property of an object? if not, why not? can I import it, export it with the @ sigil in front?).

I think making it a special kind of identifier and entity adds a lot of complexity to something that at its core should be fairly simple: a special kind of function call that lets you do things you couldn't before by making modifications to a descriptor object it is passed.

@littledan
Copy link
Member Author

@FireyFly I tried to answer that question in https://github.com/tc39/proposal-decorators/blob/cd43a46e46e242c2079ff78d144c58f478a76323/README.md#what-makes-this-decorators-proposal-more-statically-analyzable-than-previous-proposals (three questions).

@FireyFly
Copy link

@littledan oh whoops. My bad! I should've paid more attention and read more closely.

Hmm, okay, being statically analyzable is an interesting and fair point that I hadn't thought of. I could definitely see why that might be desirable.

@littledan
Copy link
Member Author

If you see any problems with it, I would be interested to hear them!

README.md Show resolved Hide resolved
PROTOSPEC.md Outdated

Similar to `#names`, there is a parallel lexical scope for `@names`. This is important so that `with` statements do not provide a way to dynamically create decorators.

Each construct which makes a new lexical scope for variables also names a lexical scope for decorators. The only type of declarations which decorators support are `const` declarations and `import { @name }` declarations.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why only const? if they can be imported, they could be exported with let and rebound later, no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, this needs to be updated to decorator @name { }. Anyway, all of these bindings are immutable to make them statically analyzable. They may be shadowed, and they may be renamed when importing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but i could export let foo = @bar, or would the only way to export a decorator be with syntax that would force it to be const?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, you couldn't do that. In the current draft, you can only define decorators as decorator @foo { }. In the previous draft, you could only use const, not let, and the name always has to begin with @, so foo would not be allowed.

PROTOSPEC.md Outdated

## Runtime representation of decorators

A decorator as applied is represented as a List of Decorator Records of the form { [[BuiltinDecorator]]: the String name of a built-in decorator, e.g., `"wrap"`, [[Arguments]]: a List of JavaScript values }.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if @ is part of the name, should it be part of this string as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, will fix (though this is editorial).


A `decorator` declaration is, effectively, a function which converts a List of JavaScript values into a List of Decorator Records.

Note, the length of the list and the [[BuiltinDecorator]] fields can be pre-computed at parse time, if the imports are parsed. Only the [[Arguments]] will differ across multiple runs of the program.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about eval?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eval would create a new sub-program, and if it has a loop inside of it, [[BuiltinDecorator]] will be consistent across that. I think this is somewhat of a detail; you can also insert more script tags or do a dynamic import to make the amount of code grow over time, and eval is just another way to do that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would eval not create a way to dynamically create decorators in the current scope, though? i'm still not clear on how that would be restricted.

Copy link
Member Author

@littledan littledan Feb 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just like lexical declarations, those would not leak out of eval. And function calls are not syntactically permitted where decorators are used (just like you can't use direct eval in a class body to create another method).


Decorator lists are evaluated into lists of built-in decorators. These are accumulated on the class in ways that are specific to each decorator:
- For `@register`, a class-wide list of calls to `register` is constructed (with the list including both the function to call and, if applicable, the property descriptor to pass to it). This list is called, in order (inside to outside, top to bottom), as the last step of creating a class.
- For `@initialize`, the field definition Record (as output by ClassFieldDefinitionEvaluation) is annotated with the callback passed, and then the callback is run each time the initializer is run. Note, only one `@initialize` call may be made per field.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why multiple registers but only one initialize?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple @registers has important use cases, e.g., unrelated pieces of metadata, but @initialize sort of logically takes over the field declaration. We could make it work, though; you could file an issue about this feature request if you can explain why you want that. I could imagine consistency arguments in either direction.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consistency and "principle of least surprise" would be mine; since i might not have any idea that the decorators i'm using is using initialize, and i explicitly slap one on the bottom, i'd expect it to override the initialization.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could imagine multiple semantics here, either removing the other @initialize call, or using its return value, etc. Sometimes we conservatively ban things when intuition can lead us astray. This is what I meant when i said I could imagine consistency arguments in either direction. It's hard to think about this abstractly, without concrete use cases in mind; feel free to file an issue on this question.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@littledan, what about multiple decorators that require instance-level initializers?
E.g., in my project, I have a system where my decorators can exchange property keys between each other. It looks like the following:

// propertyDecorator.js

import {apiMap} from './utils';

const api = (descriptor) => {
  return {
    ...descriptor,
    extras: [
      {
        initializer() {
          apiMap.set(this, desriptor.key);
        }
        kind: 'initializer',
        placement: 'static',
      },
    ],
  }
}

// classDecorator.js
import {apiMap} from './utils';

const cls = ({elements, key}) => {
  let $api;

  return {
    elements: [
      ...elements,
      {
        finisher() {
          $api = apiMap.get(this);
        },
        kind: 'initializer',
        placement: 'static',
      }
    ],
    key,
  }
}

User can apply a lot of different decorators for the single field, and many of them use this approach. The restriction "one @initialize per field" could break this system and reduce the number of opportunities previous proposal provided.

As I understand, the @initialize is the unique approach to pierce into a constructor of the class that inherits idea of the { kind: 'initializer' } of the previous proposal. If I am correct, it should have more power than to serve as a simple field initializer. E.g., we could use it for @bound decorator.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this example. Given that the finishers are static, I'm wondering, would the @register decorator work for you? If not, do you think you could give a little more context here, so I can understand the example?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@littledan, oh I see, all of what I'm talking about is proposed in NEXTBUILTINS.md.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, which of those should be prioritized, from your perspective? @register is part of the proposed MVP.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@littledan, GitHub didn't show me your answers for some reason 😁. I've created an issue with suggestion (#260) for the one thing that I consider important for my case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not, do you think you could give a little more context here, so I can understand the example?

Sorry, I'm not sure why I have chosen this example for the explanation. I thought about instance-level, but initializers in my example are completely declaration-level. I should've chosen another example.

Decorator lists are evaluated into lists of built-in decorators. These are accumulated on the class in ways that are specific to each decorator:
- For `@register`, a class-wide list of calls to `register` is constructed (with the list including both the function to call and, if applicable, the property descriptor to pass to it). This list is called, in order (inside to outside, top to bottom), as the last step of creating a class.
- For `@initialize`, the field definition Record (as output by ClassFieldDefinitionEvaluation) is annotated with the callback passed, and then the callback is run each time the initializer is run. Note, only one `@initialize` call may be made per field.
- For `@wrap`, the `@wrap` usages in methods are filtered out of the DecoratorList and called immediately when filling in the class, from the inside to the outside, before defining the method.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you elaborate on this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's unclear about it? I could elaborate if you can tell me what you want to be explained better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"inside" and "outside"; i'm not clear on what the ordering would be in, say:

@wrap(a)
class {
  @wrap(b) b() {}
  @wrap(c) static c() {}
  @wrap(d) d() {}
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, sounds like I should add an example like for @register to clarify.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added an example in the README.

@initialize(function(value, name) { this[`__internal_${name}`] = value; })
@register((target, name) => {
Object.defineProperty(target, "name", {
get() { return this[`__internal_${name}`]; },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like a perfect use case for decorator-controlled private fields.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, coming in NEXTBUILTINS.md.

new Foo().queueMethod(); // will log 1, rather than undefined
```

One possible implementation, based on `@register`:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a way to create a public field here instead of using a getter? @bound is entirely useless to me if it involves getters/setters in any way; i'd need it to put an unbound method on the prototype (which is also important for normal JS idioms of borrowable methods) and have each instance have a bound own property.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More discussion of better techniques in NEXTBUILTINS.md

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear; I consider @bound providing an own bound method an important use case for the initial iteration of decorators.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I consider @bound important as well. I've pushed NEXTBUILTINS.md; I'd be curious what you think of the two approaches listed there.

}
```

Note that a decorator like `@call` could be considered for a future built-in decorator, in a way that avoids creating an additional subclass.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this implementation allow instanceof to work? (eg, new MyDate() instanceof MyDate && MyDate() instanceof MyDate)

Copy link
Member Author

@littledan littledan Feb 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first one yes, the second one is tough. In NEXTBUILTINS.md, I'll suggest @call as a built-in decorator which does things in an even nicer way. Ultimately, this @call approach is the same as the previous decorators proposal would point towards.

- Decorators have `@` as part of their name; `@decorator` names form a separate namespace.
- There's a set of built-in decorators that serve as the basic building blocks.
- Developers to create their own decorators by composing other decorators.
- Decorators cannot be treated as JavaScript values; they may only be applied in classes, composed, exported, imported, etc.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why only classes? Since they're syntactic, they'd work on function declarations and expressions just fine, I think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that's going to be proposed in NEXTBUILTINS.md.

## Decorators
```js
decorator @xyz(arg, arg2) {
@foo @bar(arg) @baz(arg2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if, in the absence of do expressions, i want to use statements to create new arguments, based on the arguments to xyz? How would I achieve this?

Copy link
Member Author

@littledan littledan Feb 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could factor out those statements into a function, and call the function.

You raised an issue about a separate idea from @nicolo-ribaudo to allow statements in another thread. Do you have any ideas here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe instead of a decorator block consisting of nothing but decorator usages, something like this?:

decorator @xyz(a, b) {
  statement;
  list;
  with;
  normal;
  block;
  semantics;
  @one @plus @decorators @must @be @the @last @items @in @the @block?
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, let's follow up in #252 .

IMPLNOTES.md Show resolved Hide resolved
IMPLNOTES.md Show resolved Hide resolved
IMPLNOTES.md Show resolved Hide resolved
`@bound` makes a method auto-bound. By being built-in, it can have the following advantages over the `@bound` decorator described in the main README:
- Works on private methods, not just public methods.
- Can be applied to the whole class, rather than just a single method, to apply to all methods in the class.
- Can be implemented more efficiently, e.g., maintaining stable object shape, avoiding runtime metaprogramming commands, potentially tying into the object representation or the way that calls are done to make allocation lazier.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe (at least some of) the current decorator proposals allow a public field to be created, both when a method is decorated, or on the entire class for all methods, such that no getters/setters are needed.

Copy link
Member Author

@littledan littledan Feb 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, part of this is filling a gap left when making the decorators proposal more minimal, though I suspect a built-in @bound decorator will be more optimizable than the current proposal anyway.


### `@accessor`

The `@accessor` decorator creates a getter/setter pair which exposes a private field, method or accessor, as mediated by `get` and `set` callbacks. For example:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the name "accessor" strange to apply to both get and set; an accessor imo is only "get" and a mutator is only "set".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just call them @getter and @setter; there is no need to have them in the same built-in decorator: it can be otmized the same way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe @reader or @writer, even. I can think of lots of reasons for and against lots of names. PRs or issues welcome to bikeshed further.


A `decorator` declaration is, effectively, a function which converts a List of JavaScript values into a List of Decorator Records.

Note, the length of the list and the [[BuiltinDecorator]] fields can be pre-computed at parse time, if the imports are parsed. Only the [[Arguments]] will differ across multiple runs of the program.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would eval not create a way to dynamically create decorators in the current scope, though? i'm still not clear on how that would be restricted.


**Path towards features and analyzability**: Legacy decorators are run as a function, and they don't give any clear path towards being statically analyzable or expandable ahead of the time through tools, or a way to extend them to other possibilities, such as decorating field initializers, private class elements, functions, objects, etc.

**Technical infeasibility**: Legacy decorators, when applied to field declarations, depend deeply on the semantics that field initializers call setters. TC39 [concluded](https://github.com/tc39/proposal-class-fields/blob/master/README.md#public-fields-created-with-objectdefineproperty) that, instead, field declarations act like Object.defineProperty. This decision makes many patterns with legacy decorators no longer work.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick note on this, it seems like TS decorators rely on this, but Babel Stage 1 (legacy) decorators do use Object.defineProperty() for any decorated field. Strangely enough, they require loose mode for class fields currently, but don't apply the same constraint to decorated fields. This means you can't for instance decorate a field into a getter, and assign its value with the field initializer.

We're planning on using a custom babel transform for certain decorators to move field initializers into the constructor to get around this, essentially making them work like TS decorators, but figured it would be good to mention this somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, do you have a link I can include that describes the approach?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making a note that I will as soon as we get it set up!

@zloirock
Copy link

zloirock commented Mar 3, 2019

Parallel lexical scope for decorators identifiers, decorator definition not just as a syntax sugar... So, we will not be able to use one library (without transpiling this dependency) in engines with native decorators support and with transpiled decorators. Do you really think that it's a good idea?

Yes, I saw How should this new proposal be used in transpilers section and I'm definitely not a fan of this approach.

@littledan
Copy link
Member Author

@zloirock Could you be more specific about why you're not a fan of this approach with respect to transpilers?

@zloirock
Copy link

zloirock commented Mar 8, 2019

@littledan It's not always possible to use transpilers. Some examples:

  • We will not be able to load modules with decorators from third-party CDNs - who will support different versions - transpiled or not, transpiled by different transpilers since we haven't an equal based on old syntax, etc.
  • Some modules should not be transpiled. For example, transpiling core-js by babel transforms like runtime or preset-env will cause circular dependencies. I don't think that someone will configure transpiler for use only required transforms for each dependency and sup dependency.

I have enough objections to this, but this is the first thing that came to my mind.

@littledan
Copy link
Member Author

I see how this would be a big difference. What if we aim to develop a transpiler implementation before Stage 3 and try it out, to understand how it would work out in practice, and what impact these limitations have?

@zloirock
Copy link

zloirock commented Mar 9, 2019

@littledan we will be able to fully assess the problem only when native decorators implementations will be available and transpiled decorators will stop work on engines with native decorators. I don't think that many people will think about this limitation before this moment.

@littledan
Copy link
Member Author

I don't understand; why would transpiled decorators stop working in the future?

It seems to me like we'd have to:

  • Coordinate among transpilers to have a stable format for .decorators.json, as well as the runtime representation
  • Use native decorator definitions together with native decorator usages, and transpiled with transpiled

I can see how this differs from some current setups for packaging and deploying JS, but I don't understand why it's a problem.

@zloirock
Copy link

zloirock commented Mar 9, 2019

@littledan most likely a serious part of libraries will distribute already transpiled decorator and will ignore this problem since a serious part of users does not transpile dependencies. And when users will transpile their code to targets with native decorators it will stop work.

Use native decorator definitions together with native decorator usages, and transpiled with transpiled

How it could look and how simple it will be for usual users?

@zloirock
Copy link

zloirock commented Mar 9, 2019

For example, you have a simple project, just some lines with the usage of some libraries. A serious part of those libraries has own dependencies. Some dependencies should not be transpiled. Some dependencies define decorators. As an option, you also should load something with decorators definition from CDN. How you should configure your project for targes with native decorators, how for targets without native decorators?

@ljharb
Copy link
Member

ljharb commented Mar 9, 2019

Native decorators won’t be able to be relied upon in libraries for many years - top-level apps may want to use native decorators, while simultaneously using libraries that use transpiled ones (since apps can make the choice to drop older browsers far sooner than libraries can).

@littledan
Copy link
Member Author

I agree that it's really important that we get this transition story right. I think it's worth exploring in a separate issue and eventually documenting in this repository.

}
```

This example logs `"e"`, `"d"`, `"g"`, `"f"`, `"b"`, `"a"`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the most important issue I would imagine is the data transition some information with connected decorators (class -> property/method or property/method -> class). So the execution order is very important here. By default, I do the following:

const map = new WeakMap();

const cls = ({elements, kind}) => {
  return {
    elements: [
      ...elements,
      {
        initializer() { // executes BEFORE all field finishers executed
          map.set(this, someData);
        }
        kind: 'initializer',
        placement: 'static',
      } 
    ],
    kind,
  }
};

const property = (descriptor) => {
  let $data;

  return {
    ...descriptor,
    {
      finisher() { // executes AFTER all class initializer executed
        $data = map.get(this);
      },
      kind: 'initializer',
      placement: 'static',
    }
  }
}

However, with the order PR proposes it looks impossible to do this kind of trick for transfer data from the class decorator to a property/method one because property/method @register will be executed earlier than class one.

I wonder what can we do here? Set some kind of priority?

Copy link
Contributor

@Lodin Lodin Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I solved my issue. I can put things that should be initialized before the @register into a @wrap that returns the same class instance it received but with producing side effects:

const map = new WeakMap();

decorator @cls {
  @wrap(target => {
    map.set(target, someData);
    return target;
  });
}

decorator @property {
  let $data;

  @register(target => {
    $data = map.get(target);
  })
}

@littledan
Copy link
Member Author

There's still more to do on this proposal, in particular elaborate the NEXTBUILTINS much more, and prove out the tooling story (including upgrade to native), but I'm landing this PR as a step in the intended direction.

@littledan littledan merged commit a043d4b into master Mar 15, 2019
@zloirock
Copy link

Sadly.

@littledan
Copy link
Member Author

@zloirock If you have any other ideas for how to meet the goals in the new explainer, while also solving the issues you raised, please file an issue.

@zloirock
Copy link

@littledan Ok. Soon.

@ktutnik
Copy link

ktutnik commented Sep 1, 2020

@littledan I don't understand why a non JavaScript expression disallow @foo.bar?

how does it work with import star?

import * as myLib from "./my-lib" // my-lib contains custom decorators

// logically it should be @myLib.decorator? or would this be prohibited ?

My concern is this will break a lot of current implementation on libs using @Reflect.metadata()

@Reflect.metadata("key", {})
class MyAwesomeClass { }

Some frameworks uses namespace (TypeScript) to categorized decorators such as @route.get(), @val.email() its look cleaner and more intellisense friendly (from TypeScript perspective) than @routeGet() or @valEmail()

Any thought?

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

Successfully merging this pull request may close these issues.

None yet

10 participants