Static decorators: some questions / concerns based on MobX

[mweststrate]

I've checked whether the most important decorator's can be expressed in the static proposal, which raises a few questions, see the pseudo code below for more explanation:

1. Currently mobx uses existing decorators also as namespaces, would that still be possible? E.g. @action x = and @action.bound x = , @observable x = 3 and @observable.ref = {}
2. Is the ordering of decorators guaranteed? E.g. @register happens always before @initialize (if I understand the proposal correctly the answer is yes)
3. Can we target this as property target if there is no initializer expression in play? In other words, will @initializer be invoked with undefined for class X { @decorator y }?
4. @observer and observer can still be given different meaning? (if I understand the proposal correctly the answer is yes)
5. Can a decorator have different meanings for different kind of targets. Currently both @action y() {} and @action y = () => {} work in MobX
6. Double check if I understand correctly: @computed get() x {} set() x{} no longer applies @computed to the setter as is it did before?

Pseudo implementation of MobX's decorators using this proposal, giving this some context

I realize this is quite an unstructured rambling, so if there are some things unclear, I'll happily open a separate issue to discuss an individual problem:

import { action, extendObservable } from "mobx"

export decorator @action {
	@wrap(f => action(f))  // Perfect!
	@initializer((value, key) => {
		this[key] = action(value)
	})
}

// Q5: action can also be used on an initializer, and on a prototype method. Would that work with the above definition?
class X {
	@action y() {} // valid, triggers _only_ the @wrap

	@action x = y // valid as well, triggers _only_ the @initializer
}

// N.B. if @action x = y would also trigger @wrap that would be fine by me as well, but the question is what that would mean for subsequent assignments

// Q1:                       V   @action is defined above, is introducing `@action.bound`  possible?
export decorator @action.bound {
	@register((target, key) => {
		// basically the @bound example
	})
}

export decorator @observable { 
	@initialize(function (value, key) {
		this[value] = key //Q2: we can safely assume @register has already happened?
	})
	@register((target, name) => {
		// Q3: problem: this property will be enumerable, but not an *own* property
		// this can be fixed by moving the logic to @initialize,
		// but, will @initialize be invoked for fields without initializer?
		// E.g.: class X { @observable y } is quite common in mobx
		// This problem currently exists with babel as well, the getter can move the declaration to this on first read / write, but, it breaks reflection in edge cases, resulting in several bug reports in the past
                // correct ownership is usually pretty important to make sure serialization or clone utilities behave correctly
		Object.defineProperty(target, name, {
			enumerable: true, 
			get() { /* ... */ }
			set() { /* ... */ }
		})
	})
})

export decorator @computed {
	@register((target, name) => {
		Object.defineProperty(target, name, { // N.P.
			enumerable: false, 
			get() { /* ... */ }
			set() { /* ... */ }
		})
	})
}

export decorator @observer {
	@register(klass => {
		const { render } = klass
		// incorrect, but that is not the point, no immediate problem here
		klass.render = function() {
			autorun(() => this.render())
		}
                return klass

                // Or, HoC based implementation
                return class Wrapper {
                   render() {
                       return <Observer>{() => <klass />}</Observer>
                   }
                }
	})
}

// Q4: it is possible to use observer both as decorator and function, as these are distinghuisable definitions, correct?
import { @observer, observer } from "mobx"

const MyComponent = observer(() => <h1>Hi1</h1>)

@observer 
class MyComponent {
	render() {
		return <h1>H1</h1>
	}
}

Despite these questions, and it being probably impossible to maintain literally the same MobX api (seems quite a re-design is needed with this proposal), I do like the way how decorators are written :) it all does seem more explicit and intuitive.

[littledan]

Currently mobx uses existing decorators also as namespaces, would that still be possible? E.g. @action x = and @action.bound x = , @observable x = 3 and @observable.ref = {}

Not in this proposal. I wonder if we should allow this purely as a syntactic affordance, but I don't think we could allow these to be actual objects without compromising the static analyzability goals. Would something @actionBound or @observable_ref be too ugly?

Is the ordering of decorators guaranteed? E.g. @register happens always before @initialize (if I understand the proposal correctly the answer is yes)

Yes, everything is well-defined with respect to that. @initialize happens in the constructor, per instance, much later than @register.

Can we target this as property target if there is no initializer expression in play? In other words, will @Initializer be invoked with undefined for class X { @decorator y }?

A bunch of people have asked about this (e.g., #260); I think we could make it "just work", and let @initializer be used on anything to insert something into the constructor, just without a value getting passed into the callback.

@observer and observer can still be given different meaning? (if I understand the proposal correctly the answer is yes)

Yes

Can a decorator have different meanings for different kind of targets. Currently both @action y() {} and @action y = () => {} work in MobX

To some extent-- you can make @action accept both, but, for example, @register can't really tell whether it was called on a method or a field declaration. @nicolo-ribaudo raised this issue earlier.

I'm wondering if we could make some facility for conditionals within decorator declarations, but I'm a little scared to write it up, as the syntax is likely to be controversial.

Double check if I understand correctly: @computed get() x {} set() x{} no longer applies @computed to the setter as is it did before?

Right, it would just apply to the getter. Coalescing was a major source of complexity previously, and it seems unnecessary for most (but not all, see #256) use cases. I've been thinking that we could make a special built-in decorator to trigger coalescing for the cases where it's needed.

[nicolo-ribaudo]

Not in this proposal. I wonder if we should allow this purely as a syntactic affordance, but I don't think we could allow these to be actual objects without compromising the static analyzability goals.

While I don't think that it is needed in the initial implementation, something like this would be statically analyzable:

decorator @action {
	@wrap(f => action(f))  // Perfect!
	@initializer((value, key) => {
		this[key] = action(value)
	})

    decorator @bound {
      // @action.bound decorator implementation
    }
}

PS. Don't forget to always wrap decorators in backticks, or you'll ping thousands of people 😛

[mweststrate]

Would something @actionbound or @observable_ref be too ugly?

Imho, and it has the benefit of better tree-shakeable code. It will have quite some impact on the community, docs etc though.

I think we could make it "just work", and let @Initializer be used on anything to insert something into the constructor,

Yes that would be perfect, it is also the reason why the current TypeScript experimental decorators work better for fields in MobX case, then Babel legacy implementation.

I'm wondering if we could make some facility for conditionals within decorator declarations, but I'm a little scared to write it up, as the syntax is likely to be controversial.

I wouldn't go down that road to quickly. In worst case, I think we could introduce separate api's for these cases, although it is not ideal.

I've been thinking that we could make a special built-in decorator to trigger coalescing for the cases where it's needed.

If @register would also receive the original descriptor as third argument (or could obtain it through reflection), I think it could address both this case, and the @action method vs field case above. But it might be against the philosophy of the current proposal, so is hard for me to judge. Is it possible to Object.getOwnPropertyDescriptor(target, name) inside @register?

[littledan]

If @register would also receive the original descriptor as third argument (or could obtain it through reflection),

I can see how that would be helpful, and how it's useful in experimental decorators, but at the same time, I would prefer to ask people to get it through reflection at first. The reason is because it would cause performance overhead to materialize this descriptor, and most decorators don't need it.

[mweststrate]

@littledan that makes perfect sense! My first worry was that it wouldn't be available through reflection either. If it is, then there is no limitation.

[littledan]

(Note, the descriptor is only available for public class elements, not private.)

[littledan]

Thanks for the feedback; we've moved on to another proposal based on all of your feedback; see #310 and the README.