tc39/proposal-decorators

Stage 3 Criteria

pzuraq opened this issue ยท 9 comments

๐Ÿ‘

Some initial review:

  • Symbol.metadata addition should be alphabetized in the well-known symbols table
  • "The ClassElementDefinition Record Specification Type" table: s/a list/a List (so it autolinks)
  • "The ClassElementDefinition Record Specification Type" table: instead of Undefined, store ~empty~ in the empty slots
  • Table 13: Metadata Record Fields: should the records contained in the Public/Private slot lists be their own type?
  • DefineMethodProperty: "Assert: metadataList and isStatic are present" doesn't make sense when extraInitializers isn't asserted to be present. It's really weird to have 4 optional arguments to this AO in the first place, let alone where 3 of them go together. It seems like the bug is that the assertion should also assert that extraInitializers is present; but i think this AO should be redesigned a bit - it kind of seems like a separate AO would be better than drastically changing DefineMethodProperty like this.
  • CreateDecoratorAccessObject/CreateDecoratorContextObject: the enum values should be like ~foo~ and not ecmascript strings.
  • CreateDecoratorAccessObject/Create*Function/MakeAutoAccessorGetter/MakeAutoAccessorSetter: should use abstract closures instead of the older "list of steps" approach (it's likely that most of these AOs can be deleted in favor of inline abstract closures steps)

Does this mean that a consensus has finally been reached ?
Will this mean that after all mentioned above will give the green, that decorators will finally get to stage-3 ?

No; after those delegates approve the proposal spec, the chamiopn can ask to the committee consensus to move the proposal to stage 3.

No; after those delegates approve the proposal spec, the chamiopn can ask to the committee consensus to move the proposal to stage 3.

Thanks for clearing that out.

In a few days there will be a TC39 meeting and among the items on the agenda is the proposal to upgrade decorators to stage 3.

Is there any progress on the part of the delegates about the proposal revision?

syg commented

I've reviewed the proposal both as a delegate and as an editor, though owing to the size the editorial review is more cursory than in-depth.

Delegate review

As a delegate, V8 and Chrome's concern remains inclusion of the metadata stuff in this proposal. This isn't new feedback, and previously, when we've pushed back we've also gotten equal pushback from the champions that after talking with many stakeholders, the metadata use case is a crucial one for this proposal.

Mechanically at least, the metadata stuff can be separated out. As for the importance of the use case for this proposal, I really do appreciate the amount of legwork the champion group has done here in identify uses. That said, with my implementer hat on, the amount of code we'd need to ship in engines to support it does not feel right. Too much code for something that still feelsto V8should` be done in userland.

Would the champion group be open to splitting out the metadata parts?

Smaller, more specific questions/comments:

  • What's the use case for decorating fields with initializer functions again?
  • Why does metadata override the same-named property? What if you want to associate separate data between same-named fields and methods?

Editorial review

The draft spec looks pretty good! When glancing over, nothing jumped out at me, and it even uses the latest style editorial conventions of ecma262, which is a really nice touch.

@syg

Would the champion group be open to splitting out the metadata parts?

The largest concern at the moment with splitting out metadata is that a large number of decorator use cases cannot be accomplished without it. For instance, one of the most common use cases across a variety of the most use decorator frameworks is dependency injection, and eager injection essentially requires metadata.

The concern here is a pragmatic one. Decorators is in a unique space as a proposal, I would hazard to say that it is the most adopted pre-stage-3 proposal in the last several years, possibly ever. There are a lot of users and frameworks who will have to make a very tough choice if decorators ship in a partially-complete form which cannot solve basic use cases like DI (for reference, Inversify is just one decorator library whose sole purpose is DI, and it has 500k weekly downloads.) If decorators were to move to stage 4 without metadata, these users would have to choose between sticking with the current implementations and diverging from the language, or converting to a spec which cannot solve their basic use cases. The ramifications of this could be pretty severe.

However, I understand that the proposal seems large as it is and it could be difficult to gain consensus on it in its entirety. I think there's room for a few possible solutions here:

  1. We've discussed a much simpler metadata format, essentially just allowing any decorator to addMetadata which would then be associated with the class as a simple array. The downside with this approach is that basic lookups and inheritance would have to be reimplemented by each decorator, which would be a non-trivial amount of code. It also would need to be cached since it is looked up often, so it could cause memory pressure if many decorators needed to do this. Given one goal of this proposal was to focus on performance, it seemed like a better idea to ship an opinionated format which solved these concerns, but if we can't get consensus on the format and the performance concerns aren't an issue then maybe it could be simplified.

  2. If portions of the proposal need to be broken up still, then maybe we could work to ensure that each piece advances at approximately the same rate as the others. For instance, we could only move to advance the main proposal to stage 3 once all other proposals have reached stage 2. Ideally, the final advancement to stage 4 would occur all at once, in the same plenary. This would allow the ecosystem to begin adopting the new proposal in its entirety once it has reach stage 3+, while still allowing the committee to consider and design each piece independently prior to stage 4.

What's the use case for decorating fields with initializer functions again?

One use case is setting up "observation hierarchies", e.g. destroying a value when its parent class is destroyed, registering the value on the parent class via the decorator.

Why does metadata override the same-named property? What if you want to associate separate data between same-named fields and methods?

This decision making process for this goes:

  1. Inheritance is a common use case in metadata. The current structure allows us to mirror the prototype inheritance of the class within the metadata, allowing standard property shadowing for children to override parent metadata of the same name.
  2. Given this structure, we would have to have metadata for a given key always be in an array to represent multiple potential values.
  3. That would be a large number of objects to create for a very uncommon use case.
  4. It is possible for users to implement logic to collate metadata with getMetadata and setMetadata, so if necessary they can prevent the metadata from overriding.

Given all of this, this was considered the best API overall.

Thank you for the review, really appreciate it! ๐Ÿ˜„

The proposal has moved forward to stage 3, so this issue is no longer relevant. Metadata has been pulled out into a separate proposal, we'll continue the discussion on metadata there.