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

Field decorator initializer should support configurable field #523

Closed
sant123 opened this issue Feb 2, 2024 · 8 comments
Closed

Field decorator initializer should support configurable field #523

sant123 opened this issue Feb 2, 2024 · 8 comments

Comments

@sant123
Copy link

sant123 commented Feb 2, 2024

Consider this example:

function readonly(value: any, context: ClassFieldDecoratorContext) {
  return function (initialValue) {
    Object.defineProperty(this, context.name, {
      writable: false,
      value: initialValue,
    });

    return initialValue;
  };
}

class Person {
  @readonly
  firstName = "Santi";
}

const p = new Person();

This fails with the following error:

image

It happens because of this writable: false. The initializer in case it returns undefined, it should not try to set the field again. So the readonly decorator will work fine without an extra decorator in the class.

Sorry if this is duplicate but I did not find it anywhere.

Thanks.

@ljharb
Copy link
Member

ljharb commented Feb 2, 2024

Are you sure it doesn’t happen because of the implied configurable false?

@sant123
Copy link
Author

sant123 commented Feb 2, 2024

If configurable: true is provided, the returned value will replace the property entirely making readonly decorator to fail its mission:

function readonly(value: any, context: ClassFieldDecoratorContext) {
  return function (initialValue) {
    Object.defineProperty(this, context.name, {
      writable: false,
      value: initialValue,
      configurable: true,
    });

    return initialValue;
  };
}

class Person {
  @readonly
  firstName = "Santi";
}

const p = new Person();
p.firstName = "Some other value";
console.log(p);

image

@ljharb
Copy link
Member

ljharb commented Feb 2, 2024

Since classes use Define semantics, not Set, writable is irrelevant there - i think you’d have to add an initializer that runs later and marks it as nonwritable, instead of returning a function

@sant123
Copy link
Author

sant123 commented Feb 2, 2024

I'm returning a function to be able to access the this instance. Do you mean something like context.addInitializer()? That would be nice to have but it does not exists.

@ljharb
Copy link
Member

ljharb commented Feb 2, 2024

ah, right, this use case is explicitly mentioned in #437

@sant123
Copy link
Author

sant123 commented Feb 2, 2024

Interesting https://github.com/tc39/proposal-decorators/pull/437/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R370 however how can this be solved? I think if returning undefined from that function initializer should not re-assign the value to the instance; that would make the Object.defineProperty work perfectly fine.

@ljharb
Copy link
Member

ljharb commented Feb 2, 2024

I suppose you could use a class decorator that wraps the constructor and reconfigures the field at the end - otherwise, currently you can’t.

@sant123
Copy link
Author

sant123 commented Feb 2, 2024

Yeah a class decorator definitely can be of help. Also this #469 (comment) has a lot of sense for why has not been implemented this way. @ljharb thank you so much.

@sant123 sant123 closed this as completed Feb 2, 2024
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

2 participants