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

Counterintuative ordering when using accessor in combination with init methods #508

Closed
pgoforth opened this issue May 4, 2023 · 37 comments

Comments

@pgoforth
Copy link

pgoforth commented May 4, 2023

function appendString(strToAppend) {
    return function ({ set }) {
        return {
            set(value) {
                // outtermost call is applied first
                return set.call(this, `${value}${strToAppend}`);
            },
            init(initialValue) {
                // innermost call is applied first
                return `${initialValue}${strToAppend}`
            },
        }
    }
}

class FakeClass{
    @appendString('Baz')
    @appendString('Bar')
    accessor fakeField = 'foo'
}
const instance = new FakeClass()
console.log(instance.fakeField) // 'fooBarBaz'
// Setting to the same value it's initialized to, results in the opposite result
instance.fakeField = 'foo'
console.log(instance.fakeField) // 'fooBazBar'

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 as set/get accessor application. That would allow us to perform init execution in the same manner we are forced to apply set/get:

// Proposal
function appendString(strToAppend) {
    // Provide an `init` method the same as `set/get`
    return function ({ init, set }) {
        return {
            set(value) {
                // outtermost call is applied first
                return set.call(this, `${value}${strToAppend}`);
            },
            init(value) {
                // outermost call is applied first
                return init.call(this, `${value}${strToAppend}`);
            },
        }
    }
}
@pzuraq
Copy link
Collaborator

pzuraq commented May 4, 2023

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 foo === 8 in all cases, because we are effectively inlining the decorators (e.g. following the decoration pattern). If we to reverse the evaluation of initializers, it would instead be:

console.log((new A()).foo) // 8
console.log((new B()).foo) // 14
console.log((new C()).foo) // 8

Because in B, timesFour would run before subTwo. This breaks the decoration pattern, which should effectively preserve the ability to inline an inner decorator and have it be the same function.

@pgoforth
Copy link
Author

pgoforth commented May 4, 2023

@pzuraq
I just ran your example using babel and this is what I got:

console.log((new A()).foo) // 24
console.log((new B()).foo) // 24
console.log((new C()).foo) // 8

@pzuraq
Copy link
Collaborator

pzuraq commented May 4, 2023

I messed up the example, originally it was plusTwo, updated the name but not the code lol. Try now, I just ran it myself and it has the expected output.

@pzuraq
Copy link
Collaborator

pzuraq commented May 4, 2023

I'm not sure what to tell you, this is the output I'm seeing:

Screen Shot 2023-05-04 at 4 49 31 PM

@pgoforth
Copy link
Author

pgoforth commented May 4, 2023

They all evaluated to 8, it was an error on my part.

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 init and set

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 8 again right?

@pzuraq
Copy link
Collaborator

pzuraq commented May 4, 2023

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:

a.foo = 1
b.foo = 1
c.foo = 1

console.log(a.foo) // 8
console.log(b.foo) // 2
console.log(c.foo) // 4

Also note, we expect a, b, and c to have the same setters, so we should be able to set each of them to 1 and get the same value, 8 in this case. Setting them to different values is not proper inlining.

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

@pzuraq
Copy link
Collaborator

pzuraq commented May 4, 2023

@pgoforth updated my example in the previous comment, I think it illustrates the point now. Note that the ordering of C.setFoo is reflective of the ordering of the actual setters - first the outer timesFour is called, then subTwo, then finally the inner timesFour. This is what tripped me up when writing it out the first time.

@pgoforth
Copy link
Author

pgoforth commented May 4, 2023

@pzuraq What I hear you saying is that initializars cannot be composed, so in order to achieve the outcome I desire, I cannot use accessor, but instead I need to inline accessors manually like this:

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 accessor keyword is supposed to do what I've written out long form here? In reading the desugared form here, I think it accurately describes what the code above is doing. I believed that it was a shortcut for the above use case and was confused. Perhaps there's an opportunity to more accurately describe limitations and advantages of the accessor keyword, what problems it solves, and which ones it doesn't.

@pzuraq
Copy link
Collaborator

pzuraq commented May 4, 2023

@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 D(F) => F', right? The decorator does not need to know what happens inside of F, it just wraps it and produces F'. You can then wrap it again, D2(D(F)) == D2(F') == F''.

If you reverse initializer order, then this is no longer true, because the initializer of D2 runs before the initializer of D. This means that the initializer of D2 can change the behavior of the initializer of D. This breaks encapsulation.

@pgoforth
Copy link
Author

pgoforth commented May 4, 2023

@pzuraq Is there a functional equivalent to my code above, except using auto-accessors? I think the answer is "No".

@pzuraq
Copy link
Collaborator

pzuraq commented May 4, 2023

@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.

@pgoforth
Copy link
Author

pgoforth commented May 5, 2023

...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.

@pzuraq
Copy link
Collaborator

pzuraq commented May 5, 2023

@pgoforth it is perfectly possible to write decorators that do what you describe, though it is a bit complex. You can use kind to change the behavior of the decorator, and so long as the decorator is applied to the field, getter, and setter, in the same order, it should work.

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.

@pgoforth
Copy link
Author

pgoforth commented May 5, 2023

so long as the decorator is applied to the field, getter, and setter, in the same order, it should work.

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 accessor keyword does not trigger kind=field, and instead only kind=accessor. In order to initialize the field portion of the auto-accessor, I need to return an init method. All subsequent init methods receive the their initialValue from the prior (inside-out) init method. The set method on the other hand, only supplies the prior (inside-out) setter, which must be called from inside the new setter method. This results in the outermost set function being the last applied, and when executed, each inner set method is called in sequence. This results in accessor set methods resolving in outermost to innermost order, which is the inverse of the field/getter/setter decorator composition. The behavior is fundamentally different from @dec2 @dec1 set func() {} where each subsequent decorator composes the setter.

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 type ClassAutoAccessorDecorator spec in order to make auto-accessors gain the abilities I'm talking about here?

@pzuraq
Copy link
Collaborator

pzuraq commented May 5, 2023

The behavior is fundamentally different from @dec2 @dec1 set func() {} where each subsequent decorator composes the setter.

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 [[Define]] semantics. They would run the initializers, then set that value to the field, triggering any set functions. Also you could do things like lazily run the initializer, since it was captured.

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.

@pgoforth
Copy link
Author

pgoforth commented May 8, 2023

Screenshot 2023-05-08 at 1 18 57 PM

Let me know if I'm offbase with my understanding of the ordering in the graphic here. It seems like the issue of field initializations producing a different result than accessor setters could be solved if we could compose `init` and initializers instead of only reducing them.

@pzuraq
Copy link
Collaborator

pzuraq commented May 8, 2023

The problem is, intuitively, you would expect initializer modifications to run from the innermost decorator to the outermost decorator. Particularly for fields, forgetting about accessor for a moment, there is no setter, right? The only thing there is is an initializer. And given that decorators run from innermost to outermost, the expectation most people would have is that field initializers run from innermost to outermost, with the inner value being passed to the outer value.

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 accessor where it's a composition of a field and getters/setters. The field part, e.g. the initializers, would intuitively work the same as a standard field. The getter/setter part would intuitively work the same as a standard getter/setter. The only reason to reverse the order for the field is so we can have it match the runtime order of a setter. So, we would have to have a different mental model here - accessor is just like field, except this one tiny difference. I find that very inconsistent and I also don't think it would make it through review.

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 accessor is a new construct anyway, so I think we can make that work.

@justinfagnani
Copy link

intuitively, you would expect initializer modifications to run from the innermost decorator to the outermost decorator

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:

new A().x === 0;
new B().x === false;

Because I do think of this somewhat as an assignment of the initial value through the decorators.

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 accessor is a new construct anyway, so I think we can make that work.

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.

@pzuraq
Copy link
Collaborator

pzuraq commented May 8, 2023

Because I do think of this somewhat as an assignment of the initial value through the decorators.

Hmm, I can understand that logic, but I do feel like it requires some explanation to figure it out.

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.

Would this not be solvable via a flag?

@ljharb
Copy link
Member

ljharb commented May 8, 2023

Hmm, I definitely don't agree with @justinfagnani's intuition - i'd expect A to coerce to 0, and then to false - and B to be 0, just like x = falsyToBoolean(stringToNumber('0')) etc.

@justinfagnani
Copy link

Would this not be solvable via a flag?

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?

@pzuraq
Copy link
Collaborator

pzuraq commented May 8, 2023

@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);
    }
  }
}

@justinfagnani
Copy link

@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:

class A {
  @falseyToBoolean
  @stringToNumber
  x = '0';
}
const a = new A();
const initialX = a.x;
a.x = '0';
a.x === initialX;

This should be true for accessors, but I think the order should be the same for fields for consistency.

@justinfagnani
Copy link

@pzuraq that's the situation we're in today with legacy decorators, and it doesn't work consistently for fields with and without initializers.

@pzuraq
Copy link
Collaborator

pzuraq commented May 8, 2023

@justinfagnani the current spec stipulates that fields/accessors are always initialized (e.g. if there is an init function returned from a decorator, it always runs, regardless of if there is an initializer defined in code). The logic was that this could be used for decorators that want to add a value to a field (e.g. dependency injection). I think we would keep the same behavior for running setters, if we added that, and if there was no initializer then they would receive undefined. At the least, we would definitely run setters if there was an init returned from a decorator, since the value could be anything at that point.

@ljharb
Copy link
Member

ljharb commented May 8, 2023

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.

@pzuraq
Copy link
Collaborator

pzuraq commented May 8, 2023

@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.

@pgoforth
Copy link
Author

pgoforth commented May 9, 2023

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:

My main thing is that I'd expect the effect on the initial value and a set value to be the same.

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:
I feel like perhaps there could be a case where the setter could be called with the initialized value on auto-accessor instances, and leave the field behavior alone. That would give flexibility to users who decorate fields/setters separately, but provide benefit for users who want to use auto-accessors and have their initialized values equal the same value as if they set the property through the setter...kind of like an "opt-in" if you use auto-accessors:

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 initialValue through the setter, the init logic would change in the following way:

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
}

@pzuraq
Copy link
Collaborator

pzuraq commented May 16, 2023

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!

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented May 17, 2023

@pzuraq Was consensus only on reversing the order of field initializers, or of initializers in general? (i.e. also initializers added through .addInitializer)

EDIT I should have re-read the notes from yesterday before asking:

  • Consensus on option 1: To reverse the initializer order for fields and accessors, conceptually treating them like an update to the value, the same order that setters run in.

@pzuraq
Copy link
Collaborator

pzuraq commented May 17, 2023

Correct, this was just for the init initializers. I think conceptually, addInitializer initializers aren't about setting so much as finishing some setup/replacement that was supposed to occur, but has to happen per-instance rather than on the prototype. For instance, @bound needs to bind the method. So these transformations should still happen from innermost -> outermost. In addition, it gives authors some more flexibility in ordering.

This has been updated in the spec and implementations so I'm going to close this issue now.

@pzuraq pzuraq closed this as completed May 17, 2023
@trusktr
Copy link
Contributor

trusktr commented Jun 20, 2023

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 @double @triple foo = 123, regardless if during initialization or setting, I think of the first result A as A = @triple (value), and then the result B as @double (A).

This is intuitive and consistent with function calls throughout the language, and if you think about decorators, the @double decorator seems to be decorating the already-@tripleed foo property.

I.e. if we have this,

@triple
foo = 123

then adding @double like this,

@double
@triple
foo = 123

seems to be decorating the tripled foo value. The opposite is not so intuitive.

@pzuraq
Copy link
Collaborator

pzuraq commented Jun 20, 2023

@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.

@trusktr
Copy link
Contributor

trusktr commented Jun 21, 2023

@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:

From #508 (comment)

a decorator is a function D(F) => F', right? The decorator does not need to know what happens inside of F, it just wraps it and produces F'. You can then wrap it again, D2(D(F)) == D2(F') == F''.

From #508 (comment)

intuitively, you would expect initializer modifications to run from the innermost decorator to the outermost decorator

And just like @ljharb said:

From #508 (comment)

just like x = falsyToBoolean(stringToNumber('0')) etc.

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 setters before the plenary was not necessarily their order, but that they were opposite of the intuitive initializer ordering.

Now both initializers and setters are counterintuitive with the new ordering, especially for the end user of decorators, as if the '0' in falsyToBoolean(stringToNumber('0')) would be first passed into falsyToBoolean, and then the result of falsyToBoolean passed into stringToNumber.


I also agree with @justinfagnani's sentiment

From #508 (comment)

I'd expect the effect on the initial value and a set value to be the same. ie:

class A {
  @falseyToBoolean
  @stringToNumber
  x = '0';
}
const a = new A();
const initialX = a.x;
a.x = '0';
a.x === initialX;

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 set order is currently limited to outside-in (in contrast to initializers being inside-out). In fact, swapping the order of initializers may be the most minimal possible change to the spec, otherwise changing nothing else. But is it the most ideal?

I imagine the following has already been discussed: why can't auto-accessor set be applied in the same way as initializers, f.e. just return { set(v) { return transform(v) } } and return { set(v) { return this.transform(v) } }? Briefly, what functionality will this block?


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 timeThree(minusTwo(timesFour(v))). This is quite different from traditional accessors in a prototype chain: @a @b @c accessor foo is much closer in concept to nested function calls where o.foo = 2 is seems like it should be similar to a(b(c(2)) every time.

@pzuraq
Copy link
Collaborator

pzuraq commented Jun 21, 2023

@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.

@SirPepe
Copy link

SirPepe commented Aug 3, 2023

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"

Babel

Event though m1 and m2 are obviously not remotely the same thing, they both feel like methods to me, and I tend to switch from methods to class fields quite often (and without thinking too much) when I need something with a bound this. Because the use cases (with class field functions) are so similar, I think that decorators that work with both methods and class field functions are quite a reasonable idea, but now that appears to be next to impossible.

@pzuraq
Copy link
Collaborator

pzuraq commented Aug 3, 2023

@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 addInitializer still run in the original order, which allows you to use them + the access API to set the initial value in the way you want. However, there is a Babel bug, addInitializer is not available on context for fields (this was updated in the spec and should be the case).

The other bug is a spec bug. Right now, all initializers added via addInitializer run immediately after parent class fields are defined, before own class fields are defined. This means that the original function is not yet available to wrap.

I believe this should be changed, the idea behind extra initializers added via addInitializer was that they would always run before any subsequent code could read them, so that no class code could ever see the uninitialized value. So for instance, class methods have to have all of their initializers run immediately on construction because a otherwise field could read a method before it was initialized, since it is defined on the prototype:

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.

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

No branches or pull requests

7 participants