-
Notifications
You must be signed in to change notification settings - Fork 108
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
Counterintuative ordering when using accessor
in combination with init
methods
#508
Comments
As noted in the other threads where we have discussed this, the current ordering is logically consistent in that inner decorators are applied before outer decorators in all ways. Here's another example trying to explain why your suggestion doesn't work. Consider the following. const timesFour = () => {
return {
init: (v) => v * 4,
}
}
const subTwo = ({ set }) => {
return {
init: (v) => v - 2,
}
}
class A {
@timesFour @subTwo @timesFour accessor foo = 1;
}
class B {
@timesFour @subTwo accessor foo = 1 * 4;
}
class C {
@timesFour accessor foo = (1 * 4) - 2;
}
console.log((new A()).foo) // 8
console.log((new B()).foo) // 8
console.log((new C()).foo) // 8 The expected result should be that console.log((new A()).foo) // 8
console.log((new B()).foo) // 14
console.log((new C()).foo) // 8 Because in |
@pzuraq console.log((new A()).foo) // 24
console.log((new B()).foo) // 24
console.log((new C()).foo) // 8 |
I messed up the example, originally it was |
They all evaluated to Perhaps it would be better to start from a place of "what is supposed to happen", rather than "what does happen". In a vaccuum, adding a decorator to an accessor, or a field, and only operating on one half of that equation is not something I find problematic. The part that is confusing is that they behave the opposite of the expected way when operating on both I'll elaborate your example. Without running it, what would be the expected output for the last three console logs: const timesFour = ({ set }) => {
return {
init: (v) => v * 4,
set(v) {
set.call(this, v * 4)
},
}
}
const subTwo = ({ set }) => {
return {
init: (v) => v - 2,
set(v) {
set.call(this, v - 2)
},
}
}
class A {
@timesFour @subTwo @timesFour accessor foo = 1
}
class B {
@timesFour @subTwo accessor foo = 1 * 4
}
class C {
@timesFour accessor foo = 1 * 4 - 2
}
const a = new A()
const b = new B()
const c = new C()
console.log(a.foo) // 8
console.log(b.foo) // 8
console.log(c.foo) // 8
a.foo = 1
b.foo = 1 * 4
c.foo = 1 * 4 - 2
console.log(a.foo) // ?
console.log(b.foo) // ?
console.log(c.foo) // ? You would expect them to all be |
No, I would not, because that would not be following the principle of substitution/inlining properly. Your example inlines the initializer for each value, but not the setter. So, the expected result would be:
Also note, we expect Accessors don't really have an easy way to show this, so I'm going to have to break your example into two parts - the setter and the field. This is what the proper inlining of the decorators would look like. const timesFour = (m, { kind }) => {
if (kind === 'field') {
return (v) => v * 4;
} else {
return function() {
m.call(this, v * 4);
}
}
}
const subTwo = (m, { kind }) => {
if (kind === 'field') {
return (v) => v - 2;
} else {
return function() {
m.call(this, v - 2);
}
}
}
class A {
@timesFour @subTwo @timesFour foo = 1;
@timesFour @subTwo @timesFour set setFoo(v) {
this.foo = v;
}
}
class B {
@timesFour @subTwo foo = 1 * 4;
@timesFour @subTwo set setFoo(v) {
this.foo = v * 4;
}
}
class C {
@timesFour foo = (1 * 4) - 2;
@timesFour set setFoo(v) {
this.foo = (v - 2) * 4;
}
}
const a = new A()
const b = new B()
const c = new C()
console.log(a.foo) // 8
console.log(b.foo) // 8
console.log(c.foo) // 8
a.setFoo = 1
b.setFoo = 1
c.setFoo = 1
console.log(a.foo) // 8
console.log(b.foo) // 8
console.log(c.foo) // 8 |
@pgoforth updated my example in the previous comment, I think it illustrates the point now. Note that the ordering of |
@pzuraq What I hear you saying is that initializars cannot be composed, so in order to achieve the outcome I desire, I cannot use const timesFour = (m, { kind }) => {
const convert = (v) => v * 4
switch (kind) {
case 'field':
return (v) => convert(v)
case 'setter':
return function (v) {
m.call(this, convert(v))
}
}
return m
}
const subTwo = (m, { kind }) => {
const convert = (v) => v - 2
switch (kind) {
case 'field':
return (v) => convert(v)
case 'setter':
return function (v) {
m.call(this, convert(v))
}
}
return m
}
class A {
@timesFour @subTwo @timesFour #foo = 1
@timesFour @subTwo @timesFour set foo(v) {
this.#foo = v
}
get foo() {
return this.#foo
}
}
class B {
@timesFour @subTwo #foo = 1 * 4
@timesFour @subTwo set foo(v) {
this.#foo = v
}
get foo() {
return this.#foo
}
}
class C {
@timesFour #foo = 1 * 4 - 2
@timesFour set setFoo(v) {
this.#foo = v
}
get foo() {
return this.#foo
}
}
const a = new A()
const b = new B()
const c = new C()
console.log(a.foo) // 8
console.log(b.foo) // 8
console.log(c.foo) // 8
a.setFoo = 1
b.setFoo = 1 * 4
c.setFoo = 1 * 4 - 2
console.log(a.foo) // 8
console.log(b.foo) // 8
console.log(c.foo) // 8 I guess my question now becomes, am I wrong in thinking that the |
@pgoforth no, not at all. My example about inlining is just to illustrate what's happening under the hood, and why your understanding about ordering was not correct. Look, another way to explain it, a decorator is a function If you reverse initializer order, then this is no longer true, because the initializer of |
@pzuraq Is there a functional equivalent to my code above, except using auto-accessors? I think the answer is "No". |
@pgoforth well, no, because in the example in last comment you're inlining things... you can't desugar with accessors, unless we get the grouped accessor proposal through: https://github.com/tc39/proposal-grouped-and-auto-accessors But in general, yes, you can do basically everything you'd normally want to do with decorators. They are fully composable, and even in your original example repo you show it's possible to write the decorator you want to write. |
It's actually only possible if that decorator is the last one composed. If there is another, different, decorator in the chain...all previous changes are lost. So in the example above, where we are composing different decorators, the solution would not work. As our discussion as gone on, I understand that ordering is not the problem (so the title of this issue would need to be renamed when I finally am able to put my problem into words). The issue is that I cannot write a decorator that is functionally equivalent using auto-accessors (and distribute it in any kind of library for re-use by others) without mandating that they not use auto-accessors. If I'm a developer writing an third-party library exporting decorators. I should be able to write my decorators in such a way that anyone else who uses them can use them on a getter/setter pattern that is functionally quivalent to the same auto-accessor pattern, and arrive at the same result: const timesFour = (m, { kind }) => {
// ... code here
}
const subTwo = (m, { kind }) => {
// ... code here
}
class A {
@timesFour @subTwo #foo = 1 * 4
@timesFour @subTwo set foo(v) {
this.#foo = v
}
@timesFour @subTwo get foo() {
return this.#foo
}
@timesFour @subTwo accessor bar = 1 * 4
}
const a = new A()
console.log(a.foo) // 8
console.log(a.bar) // 8
a.foo = 1 * 4
a.bar = 1 * 4
console.log(a.foo) // 8
console.log(a.bar) // 8 I do not want to make a decorator library and have to tell people "You cannot use these decorators on auto-accessors, because they don't produce the same result" Auto-accessors have been given this heir of being syntactic sugar on the getter/setter pattern above, but functionally they behave very differently when it comes to decorators. So much so that I'm starting to believe they should be removed from this proposal unless they can be made functionally equivalent. I did read through the grouped decorator proposal and feel like it's much more appropriate in-leiu of this proposal's implementation. It will become confusing for developers implementing decorator libraries to have to restrict usage on something that is released alongside the same proposal they are utilizing. I did see an issue related to this here. |
@pgoforth it is perfectly possible to write decorators that do what you describe, though it is a bit complex. You can use The reason auto-accessors are included in this proposal was as a compromise. Decorators are extremely commonly used today, and many of their uses cases for fields require the ability to intercept access. In order to provide an upgrade path for all libraries, we needed to have this capability. Without it, the proposal would not have advanced to stage 3. Changes of this nature in stage 3 also have a very, very high threshold to be considered. The feature basically has to be fatally flawed in order to be changed. So far, nothing in this conversation has convinced me that this is the case, and the feedback has otherwise been universally positive, so I don’t think it will meet that bar. Grouped and auto-accessors can continue to be worked on separately, and if/when they are merged they will layer on top of this feature smoothly. |
That is the crux of the issue here. When using auto-accessors, I cannot apply the auto-accessor decorators in the same order as I do field, getter, and setter. An I do not expect anything to change related to the proposal features at this stage. I realize this is stage 3, but perhaps there is a way to accomodate this specific use case. I feel like there is a significant gap between what the proposal says it is doing, and how it is being implemented under the hood, and that is what is leading to my confusion to begin with. If there were a way to compose auto-accessors in the same way as field/getter/setter, I would be able to create decorators that work for auto-accessors in the same way they work with field/getter/setter. As it stands, their implementation is too different. Is there anything we can look at in terms of changing/adding something to the |
This is not the case, they work in exactly the same way and are functionally equivalent. I can see what you're getting at now, it is odd that you can't run the setters for the initial value of the field/accessor. This is actually how legacy Babel/TypeScript decorators did work, I'm now remembering, before fields were defined using I think this is worth discussing now that I understand your issue, but I think it will be difficult to reconcile this desired behavior with the fact that decorators typically run from innermost to outermost. I do think there is a way you could rationalize reversing field/accessor initializer order as that, but it would be very counterintuitive. I'll think on this more. |
The problem is, intuitively, you would expect initializer modifications to run from the innermost decorator to the outermost decorator. Particularly for fields, forgetting about This was how legacy initializers worked, fwiw, and I think it's unlikely we would want to change that for fields. So then we run into an issue with I think the best solution here would be to have the initial value actually trigger the setter of the accessor, like it did in legacy proposals. This would make it's behavior somewhat different than a standard class field, but |
I'm not so sure this is true. I think that for initial values I would expect modifications to happen outer-first. Imagine you have: // falseyToBoolean: converts falsey values to boolean, leave truthy value alone. x ? x : false;
// stringToNumber: converts parseable strings to numbers
class A {
@falseyToBoolean
@stringToNumber
x = '0';
}
class B {
@stringToNumber
@falseyToBoolean
x = '0';
} I would intuitively expect:
Because I do think of this somewhat as an assignment of the initial value through the decorators.
This would break a very important use-case for us, which is to be able to discriminate the initial value from "externally" set values. We need this for custom element attributes so that we don't reflect the default value of an attribute to the element. |
Hmm, I can understand that logic, but I do feel like it requires some explanation to figure it out.
Would this not be solvable via a flag? |
Hmm, I definitely don't agree with @justinfagnani's intuition - i'd expect A to coerce to 0, and then to |
Probably, any way to discriminate would suffice. But a flag on what? The setter is called, what does it check to know it's the initial value? |
@justinfagnani you could use a weakset against the instance: function attr({ set }) {
const INITIALIZED = new WeakSet();
return function(v) {
set.call(this, v);
if (INITIALIZED.has(this)) {
// do update
} else {
INITIALIZED.add(this);
}
}
} |
@ljharb I think intuition can differ here. My main thing is that I'd expect the effect on the initial value and a set value to be the same. ie:
This should be true for accessors, but I think the order should be the same for fields for consistency. |
@pzuraq that's the situation we're in today with legacy decorators, and it doesn't work consistently for fields with and without initializers. |
@justinfagnani the current spec stipulates that fields/accessors are always initialized (e.g. if there is an |
Decorators aren’t accessors, so i would expect a field to always be a field, and to be a data property - I’d expect the accessor keyword to be needed to convert it into a getter/setter. |
@ljharb yes, I don't think that's really being debated here, unless I'm misreading something. At least, that's how I see it as well. |
The issue here is that initializers (field and accessor) are always inlined, providing a new composite value to the next initializer. This makes the final value a result of innermost providing it's composite value to the outermost. Setters, on the other hand, can only be composed. This makes the final value a result of the outermost providing it's composite value to the innermost. @justinfagnani put it very simply:
We are not talking about taking away or fundamentally changing functionality, but augmenting it in a way that provides for the use case of making initializers and setters produce the same output when provided the same input. Edit: class A {
@baz
@bar
accessor x = 'foo'
// Explicitly reversing the decorators on the field to illustrate matching the auto-accessor behavior
@bar
@baz
#y = 'foo'
@baz
@bar
set y(v) {
this.#y = v
}
get y() {
return this.#y
}
}
const a = new A();
const initialX = a.x;
const initialY = a.y;
console.log(a.x === a.y); // expected: true, actual: false
a.x = 'foo';
a.y = 'foo';
console.log(a.x === a.y); // expected: true, actual: true
console.log(a.x === initialX); // expected: true, actual: false
console.log(a.y === initialY); // expected: true, actual: true If we assigned the resultant auto-accessor function dec(m, { kind }) {
switch (kind) {
case 'field':
return function (v) {
// No change
return transform(v)
}
case 'setter':
return function (v) {
// No change
m.call(this, transform(v))
}
case 'accessor': {
const { set } = m
return {
/**
* Now I don't _have_ to return an `init` that transforms the initialValue,
* because an auto-accessor will assign the `initialValue` through
* the setter, which will pick up the decorators in the expected order.
*/
set(v) {
set.call(this, transform(v))
},
}
}
}
return m
} |
We discussed this in plenary today and there was consensus to update the spec to reverse the initializers order. Thanks for bringing this up @pgoforth! |
@pzuraq Was consensus only on reversing the order of field initializers, or of initializers in general? (i.e. also initializers added through EDIT I should have re-read the notes from yesterday before asking:
|
Correct, this was just for the This has been updated in the spec and implementations so I'm going to close this issue now. |
Wow, I'm so glad this was caught for consistency. But I'm wondering, would it simply be more intuitive if both were applied inside-out instead of outside-in? When I see This is intuitive and consistent with function calls throughout the language, and if you think about decorators, the I.e. if we have this, @triple
foo = 123 then adding @double
@triple
foo = 123 seems to be decorating the tripled foo value. The opposite is not so intuitive. |
@trusktr please see the discussion notes from the plenary, this point was discussed and the conclusion was that initialization is analogous to a set, which necessarily happens in the opposite order. |
@pzuraq I understand the outcome of the May-16-plenary. I was trying to say that I agree with your initial sentiments from these comments:
And just like @ljharb said:
And I extended this sentiment: it would be intuitive if every time I set a value on a decorated auto-accessor, that the same order would apply as with pre-plenary initializers. The new ordering is counterintuitive for initializers, and what was counterintuitive about auto-accessor Now both initializers and setters are counterintuitive with the new ordering, especially for the end user of decorators, as if the I also agree with @justinfagnani's sentiment
but changing the ordering of everything else to match auto-accessor get/set feels like a regression in intuitive usage for end users of decorators. But I see the dilema: auto-accessor I imagine the following has already been discussed: why can't auto-accessor Here's an idea: with traditional accessors (the non-auto-accessor accessors (search engines please forgive us)) in a class hierarchy where each get/set pair in the prototype chain can use its own storage and still delegate to the super getter/setter, we can expand auto-accessor decorators to also have storage options. Example: // Very bikesheddable signature changes:
// Set returns the set result, *similar* to an assignment, but it returns the applied value, not the passed-in value.
// Storage in this example is opt-in, or else it all behaves like the current auto-accessor decorators.
const timesFour = ({set, get}, {storage}) => {
storage.enable() // require opt in? (throw on storage usage if not opted in?)
return {
set(v) {
v = set.call(this, v) // collect the transformed value from the inner decorator
storage.set(v * 4) // implies the return value of get.call(this) in the next outer decorator.
},
get() {
// optionally trigger the inner decorator's getter (maybe a reactive lib needs to track it as dependency)
get.call(this) // but ignore the value (we're overriding it)
return storage.get()
}
}
}
const minusTwo = ({set, get}, {storage}) => {
storage.enable()
return {
set(v) {
v = set.call(this, v)
storage.set(v - 2)
},
get() {
get.call(this)
return storage.get()
}
}
}
const timesThree = ({set, get}, {storage}) => {
return {
set(v) {
v = set.call(this, v)
storage.set(v * 3)
},
get() {
get.call(this)
return storage.get()
}
}
}
class Foo {
@timesThree
@minusTwo
@timesFour
accessor n
}
const f = new Foo()
f.n = 2
console.log(f.n) // 18 (not 16) This is similar in concept to what we can already do with traditional accessors: class A {
#n
get n() {return this.#n},
set n(v) { this.#n = v } // kinda like the private auto-accessor get/set storage
}
class TimesFour extends A {
#n
get n() {
super.n // trigger A getter (maybe it is a dependency tracking library)
return this.#n // but return our overridden value
},
set n(v) {
super.n = v
v = untrack(() => super.n) // reactive lib override has no choice but to untrack here to avoid infinite loop
this.#n = v * 4 // timesFour
}
}
class MinusTwo extends TimesFour {
#n
get n() {
super.n
return this.#n
},
set n(v) {
super.n = v
v = untrack(() => super.n)
this.#n = v - 2 // minusTwo
}
}
class TimesThree extends MinusTwo {
#n
get n() {
super.n
return this.#n
},
set n(v) {
super.n = v
v = untrack(() => super.n)
this.#n = v * 3 // timesThree
}
}
const o = new TimesThree()
o.n = 2 // timesThree(minusTwo(timesFour(2))) == 18 (not 16) At least with this opt-in storage idea, mimicking existing traditional accessor patterns on a prototype chain, it would be possible to choose between both orderings of setter calls in a chain of auto-accessor decorators. Is there some better way to achieve this, without sacrificing intuitive decorator usage on the end user side (this idea uses more memory for storage)? Because when I first saw class Foo {
@timesThree
@minusTwo
@timesFour
accessor n
} my intuition told me that every time I set n, it should be like |
@trusktr this is not changing, that is far too large of a change at this point on the process, and everyone was aligned by the end of the meeting. |
The new order of field initializers broke my brain for a few hours while I tried to write a single decorator for both methods and class fields: function append(string) {
return function(value, context) {
if (context.kind === "field") {
return function init(func) {
if (typeof func !== "function") {
throw new TypeError("Not a function");
}
return function() {
return func.call(this) + string;
};
};
} else if (context.kind === "method") {
return function() {
return value.call(this) + string;
};
}
}
}
class Test {
@append("C")
@append("B")
m1() {
return "A";
}
@append("C")
@append("B")
m2 = () => {
return "A";
}
}
let t = new Test();
console.log("m1:", t.m1()); // > "ABC"
console.log("m2:", t.m2()); // > "ACB" Event though |
@SirPepe so there are two bugs IMO that make this impossible. What should work is this: function append(string) {
return function(value, context) {
if (context.kind === "field") {
context.addInitializer(function() {
const func = context.access.get(this);
context.access.set(this, function() {
return func.call(this) + string;
});
});
} else if (context.kind === "method") {
return function() {
return value.call(this) + string;
};
}
}
}
class Test {
@append("C")
@append("B")
m1() {
return "A";
}
@append("C")
@append("B")
m2 = () => {
return "A";
}
}
let t = new Test();
console.log("m1:", t.m1()); // > "ABC"
console.log("m2:", t.m2()); // > "ACB" Initializers added via The other bug is a spec bug. Right now, all initializers added via I believe this should be changed, the idea behind extra initializers added via class Foo {
f = this.m(); // this should be bound before f is initialized
@bound m() {}
} This makes sense because methods are defined on the prototype, but fields and accessors are fully defined later. So, their initializers should run later, right after they are defined, enabling this pattern. I'll open a separate issue for this. |
I have created a minimum repro with the issue in question here. To sum up, you have to create an overly complex decorator that inverts the order of the setter calls in order to maintain the original order of the initializer calls. I hope you can appreciate how confusing it is having a simple decorator giving an incorrect result because of innermost -> outermost execution.
I would propose that the
init
methods be treated the same way asset/get
accessor application. That would allow us to performinit
execution in the same manner we are forced to applyset/get
:The text was updated successfully, but these errors were encountered: