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

Big picture #36

Merged
merged 7 commits into from Nov 11, 2015

Conversation

Projects
None yet
5 participants
@wycats
Owner

wycats commented Sep 21, 2015

This PR updates the spec with the latest thinking based on discussions with @jonathandturner

@kittens kittens referenced this pull request Oct 5, 2015

Open

The Road to 1.0 #15

9 of 18 tasks complete
providing a hook into the runtime semantics for that syntax.
If the runtime semantics for the syntax include "let x be the
result of evaluating *SomeExpression*`, that expression is passed

This comment has been minimized.

@jayphelps

jayphelps Oct 5, 2015

Contributor

I believe this is mismatched tick/double quote. Probably was suppose to be either "let x be the result of evaluating SomeExpression" or let x be the result of evaluating *SomeExpression*

type: string, // 'accessor'
hint: string, // 'getter', 'setter', 'both'
enumerable: boolean, // default: true
wrtiable: boolean, // default: true

This comment has been minimized.

@jayphelps

jayphelps Oct 5, 2015

Contributor

s/wrtiable/writable

type: string, // 'accessor'
hint: string, // 'getter', 'setter', 'both'
enumerable: boolean, // default: true
wrtiable: boolean, // default: true

This comment has been minimized.

@jayphelps

jayphelps Oct 5, 2015

Contributor

s/wrtiable/writable

```ts
interface PropertyDecoratorDescriptor<Class> extends DecoratorDescriptor<Class> {
type: string, // 'property'
hint: string, // 'shorthand' or 'explicit' or 'static'

This comment has been minimized.

@jayphelps

jayphelps Oct 5, 2015

Contributor

What happens if it's a PropertyDecoratorDescriptor that is an accessor? It appears you do not receive any hints about which ones it contains like AccessorDecoratorDescriptor's "getter" | "setter" | "both", and desc.type !== "accessor". Seems inconsistent and confusing. If you instead return AccessorDecoratorDescriptor then they miss out on the hint of whether it's static.

const obj = {
  @foo
  get bar() {}
};
@rbuckton

This comment has been minimized.

rbuckton commented Oct 5, 2015

A few years ago, when I first discussed decorators with @wycats, I started with something very similar to the DecoratorDescriptor (nee. DecoratorMetadata), and was talked down from it. Is there a reason why we suddenly need this more complicated form of a property descriptor?

It seems like it might be simpler to continue to use Property Descriptors, and have some helper APIs on Reflect to get information about the kind of thing being decorated, via context.

// just using property descriptors
function dec(target, key, descriptor) {
  if (typeof descriptor === "object") {
    if (typeof descriptor.get === "function" || typeof descriptor.set === "function") {
      // accessor
    }
    else if (typeof descriptor.value === "function") {
      // method
    }
    else {
      // field
    }
  }
  else if (typeof target === "function") {
    // class constructor or function decorator
  }
}

// with an api
function dec(target, key, descriptor) {
  let kind = Reflect.getDescriptorKind(descriptor);
  if (kind === "accessor") {
    // accessor
  }
  else if (kind === "method") {
    // method
    // or possibly "data" since we already distinguish accessor properties vs data properties in the spec,
    // but then you'd have to special case a "data" property that has a function for its value.
  }
  else if (kind === "initializer") {
    // property
    // get initializer via `target[Symbol.classProperties][name]`?
    // or descriptor.initializer?
  }
  else if (typeof target === "function") {
    // class constructor/function
  }
}

After so much time being convinced that a new, specialized descriptor is a bad idea, this feels like a step backwards.

@rbuckton

This comment has been minimized.

rbuckton commented Oct 6, 2015

@wycats Is there an up to date strawman/proposal for private slots with any information as to the requirements for private slots around information disclosure? I thought the goal for private slots was to have private state that doesn't leak without some explicit public API from the class (such as an accessor around the private state). While a decorator generally could be thought of as explicit, I worry that providing any mechanism to read/write the private state via the descriptor passed to the decorator could be considered a risk in the eyes of the folks who want to avoid avenues of disclosing private state, since those decorators are external to the class body and have the possibility of being hijacked.

Without a clear direction on the goals for private slots, I'd be somewhat apprehensive about designing decorators around a feature that may be explicitly disallowed.

@chadhietala chadhietala referenced this pull request Oct 7, 2015

Closed

Compatibility with ES6 class syntax #10341

3 of 4 tasks complete

wycats added a commit that referenced this pull request Nov 11, 2015

@wycats wycats merged commit caf8f28 into master Nov 11, 2015

@silkentrance

This comment has been minimized.

silkentrance commented Dec 3, 2015

@wycats will this be made available publicly soon? I am considering migrating to babel 6.x...

@jayphelps

This comment has been minimized.

Contributor

jayphelps commented Dec 3, 2015

@silkentrance this repo contains no actual code, it's just the draft specification unrelated to Babel.

You can track progress on Babel 6 supporting decorators here https://phabricator.babeljs.io/T2645

Someone released a compatibility plugin in that thread as well.

@trikadin

This comment has been minimized.

trikadin commented Dec 4, 2015

@silkentrance you can use this plagin while the new version of this spec (and the new babel plugin) is not ready:
https://www.npmjs.com/package/babel-plugin-transform-decorators-legacy

@silkentrance

This comment has been minimized.

silkentrance commented Dec 4, 2015

@trikadin thanks for the pointer. will use this for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment