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

Add target class to the context #517

Open
xaviergonz opened this issue Nov 12, 2023 · 6 comments
Open

Add target class to the context #517

xaviergonz opened this issue Nov 12, 2023 · 6 comments

Comments

@xaviergonz
Copy link

xaviergonz commented Nov 12, 2023

Is there a way to access the target class (not the instance) in the context (method, field, etc)?

I've been implementing the new decorators in my library and I found out the only way I have to access the target class is by adding an initializer and then getting the constructor from the instance (thus taking the initializer slight performance hit for every construction), but I'm wondering if there's any technical reason on why that target class couldn't be included in the context in the first place.

@trusktr
Copy link
Contributor

trusktr commented Nov 20, 2023

No, this is explicitly part of the design, otherwise a field decorator could do anything like modify the prototype of a class.

That's what legacy decorators proposal allowed, and browser implementers did not want to implement legacy decorators because they said that giving that much power to field decorators would cause performance problems if a field decorator could unexpectedly change the shape of a class, as the parser/compiler would see one shape, and then if a decorator changed the shape of the class, structures would have to be re-created for the new shape and cause a performance hit.

With latest decorators, the shape of the class after member decorators is the shape that you get after the class definition, before the class decorators runs. This allows the engine to guarantee that the shape of the descriptors that it creates is exactly as expected without having to re-create them.

Because of that possible performance hit, browser implementors would not agree to move the old spec forward, so this new spec prevents what you're asking about from being possible.

@trusktr
Copy link
Contributor

trusktr commented Nov 20, 2023

But! There's a way around it, if you also use a class decorator:

const decoratedProps = new Set()

function classDeco(Class) {
  const props = [...decoratedProps]
  decoratedProps.clear()

  for (const prop of props) {
    Object.defineProperty(Class.prototype, prop, { /* ... convert it into an accessor, for example ... */ })
    Object.defineProperty(Class.prototype, "somePrefix" + prop, { /* ... define a new property too, etc ... */ })
  }

  return class extends Class {
    constructor(...args) {
      super(...args)
      for (const prop of props) delete this[prop]
    }
  }
}

function fieldDeco(_, context) {
  if (context.kind !== 'field') throw new Error("fieldDeco is only for class fields")
  decoratedProps.add(context.name)
}

@classDeco
class Example {
  @fieldDeco foo = 123
  @fieldDeco bar = true
}

The above code is naive and not handling some edge cases:

  • it is not considering nested class definitions (f.e. @classDeco class Foo { @fieldDeco foo = @classDeco class Bar {...} })
  • you could forget to put classDeco on a class and so the next class with classDeco will have extra properties to handle (but you could add a lint rule for this, and you could detect when it happens in some cases but not all and throw an error),
  • etc (make sure your code is well-tested)

You can also use decorator metadata to store info.

The above workaround also introduces a performance hit due to changing the shape of the class, but for your use case (or many use cases in general) it may not matter and could be considered a "premature micro optimization". I personally have not had issues with it, and my bottlenecks have been in handling significant amounts of data in business logic or in rendering.

@aimozg
Copy link

aimozg commented Apr 14, 2024

I understand the reasoning, but this greatly complicates use cases when I need to annotate a class without transforming it.

For example, mark serializable fields of a class to create a runtime-readable structure:

@SerializableClass(/*clsid = */ 1)
class Person {
    @SzField(/* id = */ 2)
    name = "uninitialized";
    @SzField(/* id = */ 34)
    age = -1;
}
// expected runtime result
GlobalClassRegistry = {
  '1': {
    constructor: Person.constructor,
    fields: [
        { id:  2, name: "name" },
        { id: 34, name: "age"  }
    ]
  }
}

In "draft" decorators (TypeScript --experimentalDecorators) I could implement these decorators it in a way so that they run only once after class definition, and can access the class from field decorators.

In "release" decorators, I have to use hacks to glue field and class decorators together. Additionally the field decorators would produce unneeded return (initialValue)=>initialValue initialization code.

@bigopon
Copy link

bigopon commented Apr 15, 2024

If the class prototype must be protected so strongly, maybe we can exclude it during field decorator phase and only add it back after that?

@shrinktofit
Copy link

I don't believe that if we do not modify class prototype in decorator then the "engine implementators is able to determine the class shape". Developers can entirely do MyClass.prototype.fn = ... at the end of class definition. I' don't know why they think this is unacceptable in case of decorator.

@rbuckton
Copy link
Collaborator

This may be addressed by #465, but probably only as a follow-on to this proposal as this proposal is already at Stage 3.

I understand the reasoning, but this greatly complicates use cases when I need to annotate a class without transforming it.

The Decorator Metadata proposal is meant to address this scenario. You can either write public metadata to context.metadata, or use context.metadata as a key in a WeakMap. The value of context.metadata is unique to each evaluation of a class declaration, but is shared amongst all decorators for that class:

const weakSerializerInfo = new WeakMap();

function getOrCreateSerializerInfo(metadata) {
    let info = weakSerializerInfo.get(metadata);
    if (!info) {
      info = { fields: {} };
      weakSerializerInfo.set(metadata, info);
    }
    return info;
}

function getSerializerInfo(instance) {
  let metadata = instance?.constructor?.[Symbol.metadata];
  while (metadata && typeof metadata === "object") {
    const info = weakSerializerInfo.get(metadata);
    if (info) return info;
    metadata = Object.getPrototypeOf(metadata);
  }
}

export function SerializableClass(clsid) {
  return function (target, context) {
    if (context.kind !== "class") throw new TypeError();
    const info = getOrCreateSerializerInfo(context.metadata);
    info.clsid = clsid;
  };
}

export function SzField(id) {
  return function (target, context) {
    if (context.kind !== "field" || context.private || context.static) throw new TypeError();
    const info = getOrCreateSerializerInfo(context.metadata);
    info.fields[context.name] = { id };
  };
}

export function getSerializableClassId(instance) {
  return getSerializerInfo(instance)?.clsid;
}

export function getSerializableFieldId(instance, field) {
  return getSerializerInfo(instance)?.fields[field]?.id;
}

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

6 participants