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

Feedback for ES decorator proposal #223

Open
yyx990803 opened this issue Feb 13, 2018 · 13 comments
Open

Feedback for ES decorator proposal #223

yyx990803 opened this issue Feb 13, 2018 · 13 comments

Comments

@yyx990803
Copy link
Member

yyx990803 commented Feb 13, 2018

I've been contacted by @wycats (who is the champion of the ES decorator proposal at TC39) to provide feedback for the proposal from framework authors' perspective, and since this library (and vue-property-decorators) is the primary use case where Vue leverages decorators, I'm hosting the thread here.

I know there are a few discrepancies between ES and TS decorators, but am not 100% clear on the details. Ideally, I hope we can use vue-class-component and vue-property-decorators in both ES and TS contexts.

Would appreciate thoughts from @ktsn and @kaorun343 on this topic.

@ktsn
Copy link
Member

ktsn commented Feb 14, 2018

Disclaimer: I'm not familiar with the latest decorator spec yet and vue-class-component still use stage 1 decorator, so the following may not be applicable on the current spec.

TL;DR
It would be great for vue-class-component and vue-property-decorators if ES decorator does not initialize (overwrite) foo by undefined in the following case.

@Component
class MyComp extends Vue {
  @Prop foo;
}

The major difference between ES (babel legacy decorator) and TS decorators (--experimentalDecorators) is the behavior of class property initialization when there is no initializer. for example:

// Define Vue component
@Component
class MyComp extends Vue {
  // Define component props
  // props are passed from a parent component, (e.g. <MyComp :foo="'some value'" />)
  // so we should not initialize its value here.
  @Prop foo;
}

On the above code, ES decorator initializes foo as undefined in constructor while TS decorator does not do anything for foo. This ES decorator behavior can cause a problem in some cases.

Case 1: Component prop unexpectedly overwritten with undefined

@Component
class MyComp extends Vue {
  // Define prop `name`
  @Prop name;

  // Define component data `message`
  // Its initial value is calculated with prop `name`
  message = 'Hello, ' + this.name;
}

In this example, this.name will be initialized by Vue as prop and expected to be used to calculate the component data message. This pattern is well seen in many Vue components and the canonical Vue API can do this.

The problem is that ES decorator overwrite name prop by undefined even though it already has a value. So message is initialized to 'Hello, undefined'. On the other hand TS decorator works as expected since it does not initialize name. This is a large blocker to include @Prop decorator in vue-class-component.

Case 2: Cannot distinguish initialized by undefined and not initialized

In vue-class-component, all class properties are collected as component data and they will be reactive properties. For this reason, we would like to distinguish these two cases, for example:

@Component
class MyComp extends Vue {
  // not initialized
  @Prop propValue;

  // Watch changes of the value and call `someFunc` after some change.
  // initialized by undefined
  @Watch('someFunc') watchedValue = undefined;

  someFunc () {
    console.log('changed:', this.watchedValue)
  }
}

In the above example, @Prop propValue; is the same as before example, we don't want initialize propValue since its value is initialized by Vue. @Watch observes changes to watchedValue and call someFunc after watchedValue is changed. We want to initialize watchedValue and let it be reactive property because it is needed to observe a property.

In this case, propValue is declared as both prop and reactive property when we use ES decorator since propValue will be initialized by undefined and be enumerable. So vue-class-component cannot distinguish whether the user just uses it for decorator or declares as a reactive value.

For now, we made undefined as a special value (not to be converted to reactive property) to avoid this problem. But there are cases the users want to initialize them to undefined #39, #211

@yyx990803
Copy link
Member Author

yyx990803 commented Feb 20, 2018

Looks like the latest decorator proposal is quite different from the earlier version (and the one TS is currently using).

The latest proposal gives more meta-programming capabilities and after looking into it, I'm pretty happy about it. Specifically there are improvements that can help with the problems we currently see in vue-class-component:

Question for @DanielRosenwasser: does TS have the goal to update/align decorators semantics with latest ES proposal once it's stabilized?

@DanielRosenwasser
Copy link

DanielRosenwasser commented Feb 20, 2018

Yes, we intend to align once the proposal stabilizes.

@HerringtonDarkholme
Copy link
Member

HerringtonDarkholme commented Feb 23, 2018

Having access to more metadata via elements should make it much easier to translate them into vanilla Vue options;

This helps decorator library very much. Currently we have to execute class constructor to collect class elements. So we have to bother to distinguish undefined/uninitialized properties. With the new API I don't think it is necessary any more. @ktsn :)

However performance might still be a concern. With the new API, the new component decorator can be written as something like:

function Component(classDescriptor) {
    let {kind, elements} = classDescriptor;
    assert(kind == "class");
    // closure here
    let data = () => {
      const d = {}
      for (const element of elements) {
        collectData(element, d)
     }
   }
    return {
      kind,
      //elements, // elements are included in Vue
      finisher(klass) { return findSuper(klass).extend({ data })} // findSuper(klass) should be a Vue class
    }
  }

One Closure is almost unavoidable. I wonder if we can change the implementation via Proxy. Internally Vue converts object option to Vue class instance and class component converts class to object again. Currently data is used to configure reactivity.

Other than implementation, I wish decorator can help us make Vue class more declarative. For example, decorators can provide a place to explicitly declare what events one component can emit.

btw, @prop still cannot support jsx typing. TS and flow requires prop-types is defined in one class field :/

@mattheiler
Copy link

Using vue-property-decorators, I would really like to see @Prop foo: string = 'bar'; where "bar" is the default value. Currently, I'm forced to write @Prop({ default: 'bar' }) foo: string;, which is unintuitive. I noticed @ktsn tried to update this library ~1 year ago for support, but rejected the changes, citing code complexity. Would anything worth mentioning here be able to support that scenario a bit better?

@ktsn
Copy link
Member

ktsn commented Mar 15, 2018

@mattheiler I think initializer of property descriptor let us implement that without complexity 🎉

When I tried to implement that feature, we needed to initialize the original class in multiple times because it is the only way we can collect property values and it may introduce unexpected behavior if the property initializer has some side effects.

On the current decorator, we can collect them via each property's initializer. So we no longer have such problem.

@ArmorDarks
Copy link

ArmorDarks commented Sep 20, 2018

Afraid to sound stupid, but after almost two years I'm still not getting the real value of decorators.

I must agree with @carlsmith coffeescript6/discuss#9 (comment) that decorators are just a fancier way to write wrappers.

And it even doesn't look natural in JavaScript, since it would be the only operator (probably, alongside the case 'value': operator), which breaking a regular JS premise to define strict scopes with {}.

It just looks ripped off from Python and alienly slapped on syntax with quite different philisophy.

In worst scenario it should be something like

@Component() {
  Class Test {
    // ....
  }
}

@yotamofek
Copy link

yotamofek commented Feb 14, 2019

In case anyone is interested in this,
here is a little wrapper I made to be able to use this library with stage-2 decorators:
https://gist.github.com/yotamofek/2ad8fd67af4ba00458fe1eaac30d7011
This is very much POC-quality, a very naive and probably non-optimal implementation, but it does allow two nifty things:

  1. Reactive data fields with initial value of undefined:
@Component
class MyComponent extends Vue {
    reactiveVar = undefined
    reactiveVar2
}
  1. Specify default values for props:
@Component
class MyComponent extends Vue {
    @Prop({type: String}) myProp = 'default value!'
}

(Requires, obviously, the babel plugin for decorators, specifying "legacy": false in its options)

@ReinisV
Copy link

ReinisV commented Feb 14, 2019

@yotamofek your implementation seems pretty cool on first look. Don't know much about stage 2 decorators so I can't comment more than that.

I'm interested though in what you mean by 'non-optimal implementation'. On the first look, your code seems to be just a simple wrapper of the existing Prop decorator. Is there really anything that can be optimized there?

@yotamofek
Copy link

Well, my assumption that it is non-optimal is as naive as my implementation itself. :)
I'm guessing a few layers of scoping and closures can be reduced by re-writing the library to use stage-2 decorators, instead of providing a sort-of abstraction layer as I did.

@yotamofek
Copy link

yotamofek commented Feb 15, 2019

Hey @wycats - so, I've been playing around a little more with "porting" this library to use stage-2 decorators, and I might have a few pairs of cents to give on the standard.
First of all, I'd love to commend you on your work - I love the OOP enhancements the ECMA standard has gotten lately, and the new decorators are probably the most complex yet most useful of the bunch.

So, two things that I think can be improved in the standard:

  1. Currently, checking if an object is a descriptor is a little contrived and not very bullet-proof (see this), would be nice to have some non-hackable way to determine whether an object is a descriptor (maybe using PrivateName?).
  2. class extends is not reflected in the standard in any way! When decorating a class, there is no easy way to see if it's extended from any other class (best thing I could come up with was this expression inside the finisher callback: klass.prototype.constructor.prototype.__proto__). Also, there is no way to intercept and change the functionality of extending: for instance, with this library, I was hoping to be able to natively use class MyComponent extends BaseComponent even if the BaseComponent is not decorated and is a "regular" Vue object. With the current implementation, the decorator is never even called if you are trying to extend from a non-constructor object. Also, I would image the finsiher for a base class should be called (recursively), after the finisher for the inheriting class, or maybe having a separate finisher prop for subclass instances, sort of how Python did it: https://www.python.org/dev/peps/pep-0487/ (and this was, for me, a big oversight in the Python OOP model until it was implemented around Python 3.6).

@Mikilll94
Copy link

@ktsn
Any update on this? In Babel 7, the plugin @babel/plugin-proposal-decorators uses stage-2 decorators as default.

@wycats
Copy link

wycats commented Aug 18, 2020

@yotamofek @Mikilll94 I've been working with @littledan, @pzuraq and the TC39 JavaScript Frameworks outreach group on a new draft of the decorators proposal. So far the initial responses have been positive and I hope we have something reviewable soon.

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

No branches or pull requests

10 participants