-
Notifications
You must be signed in to change notification settings - Fork 106
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
Why no addInitializer
for class fields?
#469
Comments
The spec actually has
They are not limited, they are sequenced.
|
Is there an explainer to the performance difference? At first thought, there doesn't seem to be any difference. |
@pzuraq I can't imagine why initializers added with I also don't see why How does that make it slower? |
@trusktr the reason has to do with optimizations that browser engines make during the initialization of a class. Specifically, browser engines are able to create a single operation (as I understand it, this is probably dramatically simplifying) for the assignment of all of the class's fields. Because class fields are knowable at parse time, this operation can be created at one time during initial compile. If Previously, initializers did act this way, but we also had the This is documented in the PR which made this change: #436 |
@pzuraq Thanks for explaining, but I still don't understand. I read #436, but it doesn't say much about those mechanics you mentioned. How is it that either of these,
has anything to do with what the engine does with the initializers after the decorator call? Either way, the engine will have the initializers at the same time, which is right after synchronous decorator execution. |
Or in other words, these two appear to lead to exactly the same result: function decorator() {
return () => 123
}
// engine calls decorator()
// engine now has initializer immediately after decorator() call function decorator(_, ctx) {
ctx.addInitializer(() => 123)
}
// engine calls decorator()
// engine now has initializer immediately after decorator() call (same as before!) |
From my understanding there are differences between the two kinds of field initializers. For example a returned initializer function (first example) is the only one defining the value for the property with its return value. Added initializer (second example) returns are ignored, those initializers having a return type of This is where I think the optimization breaks down (though I personally don't know anything about the actual optimizations). Where before you had:
With the inclusion of
You now have two places to worry about user code running rather than one.
I'm not sure where this fits in because I don't think you should be able to know class fields at parse time, at least not per instance given you could have something like: class A {
x = A.o = this
y = (() => { throw 0 })();
}
try {
new A
} catch {
console.log('x' in A.o, 'y' in A.o) // true, false
} The field |
I suggest reading this blog post for more details about the startup costs of class fields, and the types of optimizations that the Chrome team and others are taking to make them as performant as standard property assignment. While it doesn't touch much on this exact area, I think it demonstrates how this is a very important and area for optimizations, and that any extra operations we must perform here could have significant impacts at scale.
This is incorrect, the order is:
My understanding is that this is not about knowing the class fields themselves, it's about knowing when and how user code checks will have to be inserted. Again, IANA browser engine dev, but you could imagine that with this class: class C {
@dec
a = 123;
@dec
b = 123;
} With the current setup, the initialization logic looks like:
Were the initializers interleaved, it would look like:
This is an extra operation which would have to be added (e.g. you could not possibly know if there were added initializers until the decorator itself were evaluated, at run time). |
I think you may be misunderstanding how For a field decorator, the returned callback is executed as a pipeline. This allows multiple decorators to affect the initialized value: const addOne = () => value => value + 1;
const double = () => value => value * 2;
class C {
@double
@addOne
x = 1;
@addOne
@double
y = 1;
}
new C().x; // 4
new C().y; // 2 Above, the decorators for class C {
x = double()(addOne()(1)); // (1 + 1) * 2 = 4
y = addOne()(double()(1)); // (1 * 2) + 1 = 2
} However, const init = (_, ctx) => {
ctx.addInitializer(function () { // must be a `function` to get the correct `this`
console.log(this.constructor.name);
});
}
class C {
@init x = 1;
}
new C(); // prints: 'C' |
Can I ask you to explain why field |
I think that's just a typo.
They actually don't have different behavior, because in the context of the current spec, one does not exist for class fields! If we added Also keep in mind that The real question is: why is the return value, which has a DX that is inconsistent with the other type of decorators, actually better? So far, no one has been able to actually explain this, and having developer API consistency is valuable. |
I re-opened this at #473 because there has not been any legitimate explanation as to why It may be that the previous version of it was bad and was removed (instead of being modified). But currently there is no version of it, which means we can make it be as good as any version we can imagine, and we can remove the function return value. |
@trusktr why is #469 (comment) illegitimate? Clearly some members of the committee believe that the extra operations that would be required for your proposed solution are a non-starter. Your proposed behavior was discussed early in the design process and discarded for this reason. I’ve closed the other issue because the answer to it is the same as this one - we cannot have the behavior that you describe due to performance constraints. It was proposed, it was discussed, and it was turner down by the committee, for the reasons described above. |
Maybe I'm not understanding. Are you saying that with a decorator that returns an initializer, the engine knows something about the to-be-returned initializer without running the decorator at runtime? But with Are you saying that the engine will statically analyze a decorator, and it will see that there is a returned function in a decorator (before running it) and do something with that?
Because, unless there's some magic I'm not aware of,
What I'm saying is that point 1 seems completely decoupled from point 2, but you seem to be implying that they are not (and if that's the case, then I'm misunderstanding something). |
What I'm saying is, with
There is no interleaving! (I'm not proposing to allow both returning and passing-to-method. Returning would be removed in favor of passing-to-method, for consistency) How does the choice of returning an initializer vs passing an initializer to a method (there should only be one option, not both) have anything to do with whether or not the initializers are interleaved? This question is what has not been explained, unless I completely misunderstand something. |
Thank you for elaborating, as the question being asked has changed several times now and it's hard to know exactly what is still confusing. I believe what you are saying is:
This would satisfy the performance constraints and is a viable alternative to the current behavior. However, I believe that it would be an equally confusing API for a few reasons:
In addition, it is conceivable that users may want to run code during the pre-field-class-initialization step, and this change would make that impossible. While having both types of initializers may be a little confusing, it is also more powerful, and this was a requested capability by some members of the committee and the reason why I believe that ultimately this is a bikesheddable API design choice - both solutions are confusing in their own ways, and consistent in their own ways. Personally I believe the current design is the more intuitive and better one, and given this proposal has moved to stage 3 already I don't think it is likely that it will change, as changes in stage 3 are meant to only be for critical issues based on implementation experience (see the TC39 process doc). |
That's not what I'm saying and I'm not confused. The current spec states that "field" decorators do not have a
(This piece of text does not link to the "detail below", making it a bit difficult to follow.) Further down below, the Class Fields section shows the type definition for field decorators to be:
(which seems to have a mistake in the return type definition) Then it states that
So I'm not confused about class fields having
because the spec currently says that field decorators don't have both, that they have only returned initializers.
What I proposed (this whole time, sorry that I didn't explain it well previously) is, why not to remove returned initializers, and replace them with The type of a field decorator would be changed to the following:
What I'm thinking is that an initializer passed into The wording below the type def would be modified to:
etc. This would make the decorator dev experience more consistent. I don't yet understand why using |
@trusktr the explainer document you linked to is not the spec. This is the spec: https://arai-a.github.io/ecma262-compare/?pr=2417 In the spec, It was discussed in the same plenary when decorators advanced to stage 3, and the decision was that
I believe I explained my reasoning in #469 (comment). As mentioned there, the proposed behavior you put forward works technically, and I have admitted that I can see why you believe it is more intuitive. I believe that the spec'd behavior as is is more intuitive however, and I don't believe we can objectively find a reason for one being better than the other, other than it is slightly more powerful to be able to add initializers in both places in the current spec. Given the proposal has already advanced to stage 3, I don't believe any changes can be made here based on some developers finding it more or less intuitive. This goes for changes that I myself would like (I in fact made a proposal for a change at the last plenary, but it was shot down for similar reasons). |
Oops, so I mistakenly thought the explainer was up to date and somehow didn't get that above. Then both, returned initializers, and those added with I took a at |
They do not, returned initializers are added to the class element list of initializers,
Yes that appears to be a mistake, |
That's a little confusing. Now we have two ways to add what otherwise appear to be the same thing, yet they behave differently. (one is interleaved, the other isn't, right?) Why don't all class field initializers (returned or passed) behave the same? |
See the detailed reasoning in #469 (comment):
|
About the idea of allowing field decorators to add two types of initializers, one for interleaved, one for pre-field stage, why would an overloaded I think having initializers with or without Here's a practical example of what usage of an overloaded function deco() {
if (kind === "method") {
addInitializer(() => {...})
} else if (kind === "field") {
addInitializer(value => {...}, true) // pre-fields
addInitializer(value => {...}, false) // per-field
} else if (...) {...}
// ...do something else regardless of decorator type...
} where the boolean would default to one or the other. Maybe pre-fields has no With returned initializers, code gets more complicated: function deco() {
if (kind === "method") {
addInitializer(() => {...})
doSomethingRegardless()
} else if (kind === "field") {
addInitializer(() => {...}) // pre-fields
doSomethingRegardless()
return value => {...} // per-field
} else if (...) {
...
doSomethingRegardless()
}
function doSomethingRegardless() {
// ...do something else regardless of decorator type...
}
} or function deco() {
let returnInitializer = undefined
if (kind === "method") {
addInitializer(() => {...})
} else if (kind === "field") {
addInitializer(() => {...}) // pre-fields
returnInitializer = value => {...} // per-field
} else if (...) {...}
// ...do something else regardless of decorator type...
return returnInitializer // in case it was a field decorator
} The The first example with the overloaded |
Why could the code not be: function deco() {
doSomethingRegardless()
if (kind === "method") {
addInitializer(() => {...})
} else if (kind === "field") {
addInitializer(() => {...}) // pre-fields
return value => {...} // per-field
} else if (...) {
...
}
}
function deco() {
if (kind === "method") {
addInitializer(() => {...})
doSomethingRegardless()
return function() {}
} else if (kind === "field") {
addInitializer(value => {...}, true) // pre-fields
addInitializer(value => {...}, false) // per-field
doSomethingRegardless()
} else if (...) {
doSomethingRegardless()
}
function doSomethingRegardless() {
// ...do something else regardless of decorator type...
}
} The change doesn't seem to make all use cases better, just one niche use case, at the expense of making the API as a whole less consistent overall. As mentioned previously, there is detailed reasoning for the current API choices, and as this proposal is now in stage 3 changes can only be made if there are fundamental problems with the current design. |
Just a comment... Our serialization system does heavily depend on class field initializers -- we treat the initializer's evaluation result as the "default value" of that property, then, we can omit the serialization of that property if its current value is equal to the "default value". This makes users happy in most time though sometimes the evaluation brings side-effects. So, we're using babel's decorator model: the initializer, as a function, is passed to the decorator. It's unfortunate to see the new proposal does not provide the ability to access initializer from decorators. To achieve the original effect, we have to write something like: class Foo {
@serializable // Marks this field as serializable
@defaultValue(3) // Hints the "default value, we have to code the initializer again!
bar = 3;
} |
@shrinktofit I'm not sure why this wouldn't be possible with the proposal as is, you could do: const INITIAL_VALUES_MAP = new WeakMap();
function serializable(_, context) {
return function(initialValue) {
// Using https://github.com/tc39/proposal-upsert for brevity
INITIAL_VALUES_MAP.emplace(this, { insert: () => new Map() }).set(context.name, initialValue);
return initialValue;
}
}
function serialize(obj) {
const initialValues = INITIAL_VALUES_MAP.get(obj);
const serialized = {};
for (const [key, value] of Object.entries(obj)) {
if (value !== initialValues[key]) {
serialized[key] = value;
}
}
return serialized;
} This would be order dependent, The downside here is that you would have to instantiate an object per instance of the class to track initial values, but this would also be more correct and would not result in the possibility of side effects by running the initializer prematurely. |
Thanks for direction. The suggested solution actually gives per-instance default properties:
|
@shrinktofit yes, I noted this in my comment as a downside. The reason that initializers were not captured was due to different performance concerns from the browser teams. Doing so would lead to even worse performance outcomes. |
And why is a class field limited to only one initializer-returning decorator?
It is going to lead to frustration when someone tries to do this and it doesn't work:
when both return an initializer.
And if both of those should work and I read the explainer wrong (and it needs an update), then I see no reason why we can't keep the API consistent and use
context.addInitializer()
instead of a return value.The text was updated successfully, but these errors were encountered: