`context.access` API implementation feedback.

[rbuckton]

TypeScript has been working on an implementation of the Stage 3 decorators proposal, and have been evaluating a number of user scenarios leveraging the current specification text. As part of this, we have found that the current design for the context.access object's API has a somewhat poor DX, and is missing a necessary capability (access.has).

context.access.get and context.access.set DX Concerns

The API for the get and set methods on context.access currently require you to pass the this context via .call:

function dec(target, context) {
  context.addInitializer(function() {
    // get a value
    context.access.get.call(this);
    // set a value
    context.access.set.call(this, x);
  });
}

I believe this was intended to mimic the behavior for method/auto-accessor replacement, i.e.:

function methodDec(target, context) {
  return function (...args) {
    return target.call(this, ...args);
  };
}
function accessorDec(target, context) {
  return {
    get() { return target.get.call(this); },
    set(value) { target.set.call(this, value); },
  }
}

However, this may be the wrong behavior to mimic. The .call behavior is a necessity for method replacement, since both the function you are replacing and the function you replace it with must pass along this as the this-binding. However, access.get and access.set don't have that requirement, and more closely resemble Reflect.get/Reflect.set and WeakMap.prototype.get/WeakMap.prototype.set.

We have found the current metaphor, emulating replacement, is far more confusing.

context.access.has Method

Another issue we've found is that, while context.access.get and .set are extremely valuable for some scenarios where private member access is required, the fact that there is no imperative mechanism to emulate #x in obj via context.access can make it difficult for some of these scenarios to perform pre-emptive validation.

Having such an API would also more strongly inform the correct metaphor for .get and .set, in that you are far less likely to expect to do context.access.has.call(obj), given that there is no direct corollary to that in the method replacement metaphor.

Suggested Changes

As such, we would suggest the following changes to the context.access API to improve the development experience:

• Change context.access.get.call(target) to context.access.get(target)
• Change context.access.set.call(target, value) to context.access.set(target, value)
• Add context.access.has(target)

Or, in TypeScript parlance:

 type DecoratorContext = {
   kind: string;
   name: string | symbol;
   static: boolean;
   private: boolean;
   access: {
-     get(): any;
-     set(value): void;
+     get(target): any;
+     set(target, value): void;
+     has(target): boolean;
   },
   addInitializer(cb: Function): void;
 }

[justinfagnani]

context.access.get(target) instead of context.access.get.call(target) is indeed a nicer API.

[rbuckton]

Given our release cadence and the timing of the upcoming TC39 plenary, the TypeScript 5.0 beta will likely ship with Stage 3 Decorators support, with the exception of context.access. We've determined that if any code were to become heavily dependent on the current .get/.set semantics, and if those semantics were to change, it would be impossible to reliably feature test for. Whether to use .get.call(obj) or .get(obj) is dependent on the calling code, not your own code.

With how the TS emit for Stage 3 Decorators works today, enabling context.access in the future would be an emit change that wouldn't require changes to our helpers or the tslib package. Our hope is that we can get a decision on this proposed changed in the January plenary so that we can make the necessary changes prior to our 5.0 release candidate.

[pzuraq]

After some thought I agree with these changes overall, I do think it would be a better DX to not use .call even though it means developers will have to remember to use .call with methods/accessors but not with access. The thing that won me over is the parallel to Reflect.get/Reflect.set, I do think it's very natural to think of them as pretty similar in the larger JS context. I also think that either way adding .has makes perfect sense, and it would be additive so I don't believe it should be too controversial.

I am not sure if I'll be able to attend the January plenary (I just had COVID and have been catching up on a decent amount of work) but I support this being presented to the committee.

[Jack-Works]

I have raised this problem before but it looks like the designer doesn't like this idea.

I also worry that this change increases memory usage.

class T {
    @f accessor x = expr
}

function f(context) {
    // every `new T` creates a new context.access.get/set
}

[pzuraq]

@Jack-Works by the designer, do you mean me? Just confused because I'm the current champion/designer and I just commented that I support the change, sorry if I'm misunderstanding 😅

re: memory usage, the design would not require a new access to be created for each object because you would pass the object in as the first parameter, the object is not bound to the this context of each object/function.

[ljharb]

It would still require creating a new one every time, unless we wanted one decorator to be able to add properties to it, and have later decorators see those - I doubt we want that communications channel.

[pzuraq]

Hmm, good point, but then that would be the same with the current design as well though. Still I think it's better that it's unbound, there's the parallel to Reflect, and maybe one less reference to store?

[ljharb]

Yes, I tend to agree - i agree the performance/new object story is the same either way, and adding has is a super no-brainer (good catch @rbuckton et al).

[mhofman]

It would still require creating a new one every time, unless we wanted one decorator to be able to add properties to it, and have later decorators see those - I doubt we want that communications channel.

Very good point. I don't think we should allow shared mutable instances between decorators like this. But given that you'd only have to create one for each [decorator, decorated accessor] tuple, I don't think that's particularly a high cost. Furthermore engines are free to lazily create this access object on first access from the context, or to each function on first access from the access object (I assume the former makes more sense and is simpler). Should the access property of the context be made a getter to avoid engines having to go for exotic behavior?

[ljharb]

I don’t think it needs to be a getter - we don’t need to specify unobservable optimizations.

[rbuckton]

I have raised this problem before but it looks like the designer doesn't like this idea.

I also worry that this change increases memory usage.

class T {
    @f accessor x = expr
}

function f(context) {
    // every `new T` creates a new context.access.get/set
}

This is an incorrect assumption. A new access isn't created for every new T, but it is created for every decorator application. Unless held on to, these objects would essentially be nursery objects so I don't think they'd live long enough to be a memory hazard.

[Jack-Works]

@Jack-Works by the designer, do you mean me? Just confused because I'm the current champion/designer and I just commented that I support the change, sorry if I'm misunderstanding 😅

Oh when this design (access.get.call(this, x)) first PRed into the repo, I commented my concern but someone says that's how it should be, maybe that's not you 🤔

[ljharb]

Searching leads me to #452 or #450 or #310 (comment), but none of those seem to be discussing the possibility of first-arg get/set functions :-/

[pzuraq]

Oh when this design (access.get.call(this, x)) first PRed into the repo, I commented my concern but someone says that's how it should be, maybe that's not you 🤔

I do think we may have discussed this in meetings, which may be why it isn't in the issues. I did previously think that the .call design was better based on the idea that it would be consistent with the way methods/accessors are called, but the argument about the parallel to Reflect, along with real world usage and feedback, made me change my mind.

[iainireland]

It would still require creating a new one every time, unless we wanted one decorator to be able to add properties to it, and have later decorators see those - I doubt we want that communications channel.

If we froze the context and access objects, and then specified that we always reused the same frozen object for a given application of a decorator, would anything break?

[justinfagnani]

You need a new access object per {field x instance} anyway. Freezing it may be more costly - that might depend on the average number of decorators per decorated field.

[ljharb]

You wouldn't need a new one if the functions take the instance as either the receiver or an argument.

[pzuraq]

These changes have been merged into the spec, so I'm going to close this issue.

[trusktr]

Unless held on to, these objects would essentially be nursery objects so I don't think they'd live long enough to be a memory hazard.

Can someone else's decorator override my context's access properties?