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

[on hold] WIP: Support decorators stage 2 proposal, implements #1352 #1732

Closed
wants to merge 23 commits into from

Conversation

mweststrate
Copy link
Member

@mweststrate mweststrate commented Sep 20, 2018

@coveralls
Copy link

coveralls commented Sep 24, 2018

Coverage Status

Coverage decreased (-0.2%) to 93.756% when pulling 88c8c3a on decorators-stage-2 into d6f2b54 on master.

* Computed properties become now live on the instance, rather than the prototype, so they become part of the _ownProperties_ of an object. However, they are still not enumerable, so this should make no difference in practice.
* MobX generates non-enumerable utility properties that are all of the form `__mobx-initializer-${propName}`. Those always have the value `undefined` and you can even delete them if they annoy you, but those were introduced to work around some limitations of the current stage2 proposal ([background](https://github.com/tc39/proposal-decorators/issues/153))
* By supporting the stage2 implementation, it is now possible to use `@babel/plugin-proposal-class-properties` in spec mode (that is, with option: `{ "loose": false }`). However, the `spec` mode breaks using the `decorate` utility on class instances, so for now it is recommended to keep using `{ "loose": true }` in your babel configuration
* The value of te `decoratorsBeforeExport` setting of `plugin-proposal-decorators` is technically indifferent to MobX, but it is recommended to set it to `true`, this is in line with the legacy syntax and all current MobX documentation. With `true`, one can write: `@observer export class MyComponent extends React.Component {...`, with `false`, this needs to be rewritten as: `export @observer class MyComponent extends React.Component {...`. ([background](https://github.com/tc39/proposal-decorators/issues/69))

Choose a reason for hiding this comment

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

Maybe instead of recommending decoratorsBeforeExport: true you could just say that it is similar to the old behavior but that it could change? Since TC39 hasn't decided yet it would be better not to recommend neither true not false.

}

export function quacksLikeAStage2Decorator(args: IArguments): boolean {
return args.length === 1 && args[0] && (args[0].kind === "field" || args[0].kind === "method")

Choose a reason for hiding this comment

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

I think that it would be more future-proof to also check if args[0].kind === "class", just in case someone tries to use this helper with a class decorator.

@mweststrate mweststrate changed the title Support decorators stage 2 proposal, implements #1352 WIP: Support decorators stage 2 proposal, implements #1352 Nov 6, 2018
@mweststrate
Copy link
Member Author

Current main blocker: wait until tc39/proposal-decorators#165 is implemented in babel.

@nicolo-ribaudo
Copy link

I will implement it as soon as that PR is merged in the proposal repo.

@mweststrate
Copy link
Member Author

@nicolo-ribaudo awesome, thanks!

@IngwiePhoenix
Copy link

Is the decorator-plugin so much in WIP that you can not use it without legacy being turned on? I am asking because I am - sort of desparately? - trying to get class-level decorators to work, but have had little to no luck. I managed to get method decorators to work, but that's it. I can not seem to be able to attach a field to a class through a decorator.

"legacy" sounds like "old", or rather "do not use for long-term support". I would like to get started on decorators with sort of an LTS in sight, so that I can stay on that implementation for at least a while.

This is the test-file I am trying to compile:

function classDecorator(value) {
  //console.log("class", value, arguments.length, arguments)
  return function decorator(target) {
    let obj = {
      kind: "field",
      key: "isDecorated",
      placement: "static",
      descriptor: {
        configurable: false,
        enumerable: false,
        writeable: false,
      },
      method: () => {console.log("class rt mtd"); return true},
      value: true
    }
    target.elements.push(obj)
    console.log(target)
    return target
  }
}

function methodDecorator(value) {
  //console.log("method", value, arguments.length, arguments)
  return (target) => {
    //console.log("method rt", arguments)
    target.descriptor.value.isDecorated = true
    return target
  }
}

@classDecorator("class val")
class Bar {
  @methodDecorator("mtd val")
  baz() {
    console.log("foo")
  }
}

console.log(Bar, Bar.isDecorated, Bar.prototype.isDecorated)
// Expected to see the class and "true".

And this is the .babelrc:

{
  "presets": [
    ["@babel/preset-env", {
      "targets": { "node": "current" }
    }]
  ],
  "plugins": [
    ["@babel/plugin-proposal-decorators", {
      "decoratorsBeforeExport": true
    }]
  ]
}

@mweststrate
Copy link
Member Author

mweststrate commented Jan 21, 2019 via email

@mweststrate
Copy link
Member Author

Latest status: initializers haven't landed yet in babel, since the proposal was slightly changed. Waiting for https://github.com/babel/babel/pull/9360/files to land.

@mweststrate
Copy link
Member Author

Update: the proposal is undergoing significant changes again. Waiting on that: tc39/proposal-decorators#250

@mweststrate mweststrate changed the title WIP: Support decorators stage 2 proposal, implements #1352 [on hold] WIP: Support decorators stage 2 proposal, implements #1352 Mar 14, 2019
@mweststrate
Copy link
Member Author

Closing for now, will be reopened once #1928 happens.

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.

4 participants