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

Allow decorating the "constructor" as a function #419

Closed
Jamesernator opened this issue Aug 2, 2021 · 4 comments
Closed

Allow decorating the "constructor" as a function #419

Jamesernator opened this issue Aug 2, 2021 · 4 comments

Comments

@Jamesernator
Copy link

Jamesernator commented Aug 2, 2021

This is an issue to reconsider allowing decorating the constructor function directly (as per this request).

Being able to decorate the constructor function itself directly would make decorators that would usually operate on methods behave more similarly with the constructor rather than having two kinds of certain decorators.

In particular the sort've decorators of interest are ones such as param/return checking, wrapping return values (e.g. wrapping this with proxy), memoization and so on.

For methods when a wrapping decorator is applied, in general external code could not actually know of the decorator, however when creating decorators such as the above with subclass-returning-decorators each such decorator adds an extra class into the chain, in general these classes may not be desirable to be accessible. As an example for a decorator like @validateParams(isNumber, isString) under the current spec could only return a subclass, but consumers of the class could trivially access the class where this decorator is not applied by looking up the prototype e.g.:

function validateParams(...paramCheckers) {
    return function(func, context) {
        if (context?.kind === "class") {
            return class extends func {
                constructor(args) {
                    for (const [index, checker] of paramCheckers.entries()) {
                        if (!checker(args[index])) {
                            throw new TypeError("Argument is of wrong type");
                        }
                    }
                    super(args);
                }
            }
        }
        // Others omitted
    }
}

@validateParams(isNumber, isNumber)
class Point {
  #x; #y;

  constructor(x, y) {
    this.#x = x;
    this.#y = y;
  }
  
  get x() { return this.#x; }
  get y() { return this.#y; }
}

// Despite the checker we can still access the original class:
const OriginalPoint = Object.getPrototypeOf(Point);

// Oops we added the decorator to specifically prevent this
const point = new OriginalPoint("this", "works");

This is quite different to applying the decorator to any other method, or function, in general this sort've thing cannot be broken from the outside unless the decorator explictly opts in to do so.

My suggestion would be to adopt the ability to decorate the internal constructor method-esque thing directly similarly to as suggested previously.

Essentially decorating constructor would behave primarily like a method decorator but being passed a function which constructs the instance passed as a value. The function given to this decorator would be effectively like the super() "function" in a subclass except without setting the decorated function's this.

i.e. Usage would be like:

function validateParams(...paramsCheckers) {
    return function(func, context) {
        if (context?.kind === "constructor" || context?.kind === "method") {
            return function(...args) {
                for (const [index, checker] of paramsCheckers.entries()) {
                    if (!checker(args[index])) throw new TypeError(`wrong type`);
                }
                // The fact this could be a "constructor" doesn't really matter here
                // This will just construct the object and return it as per usual as if
                // this were super(...args) in a subclass (without setting `this` locally)
                return func(...args);
            }
        }
    }
}

// class-level decorators would still be useful for things like mixin behaviours and such
class Point {
  // ... Same as before except decorator on "constructor" itself
  @validateParams(isNumber, isNumber)
  constructor(x, y) {
    // ...
  }
}

// Just Object.prototype as expected
Object.getPrototypeOf(Point); 
@pzuraq
Copy link
Collaborator

pzuraq commented Aug 2, 2021

@Jamesernator I have three main concerns with moving in this direction.

  1. This could potentially add additional complexity to the construction of all classes. We would need a place to store these wrapped constructor functions, and we would need to check if they existed and call them. This proposal has been blocked before on performance concerns, so if we were to add this functionality it would have to be in a way where it has absolutely no impact on class construction.

  2. I'm not sure if there are many use cases for this type of behavior. I can understand wanting to avoid additional links in the prototype, but it seems like these use cases such as the @validateParams decorator you have here could be potentially solved better by parameter decorators, which we want to add in a future proposal

  3. This increases the scope of an already very large proposal. In fact, it has been brought up multiple times in committee that the current decorators proposal could potentially be broken down into smaller proposals for each type of decorator, in order to speed up the process. The only reason we haven't done this is because we are trying to make sure that the majority of existing use cases in the ecosystem covered, so that we don't introduce a feature that is difficult or impossible to transition to for users of the existing decorators implementations. This is also why the accessor keyword is still part of this proposal, for instance.

Given these considerations, I think that the best path forward would be to create a follow-on proposal for this functionality.

@Jamesernator
Copy link
Author

Jamesernator commented Aug 2, 2021

  1. This could potentially add additional complexity to the construction of all classes. We would need a place to store these wrapped constructor functions, and we would need to check if they existed and call them. This proposal has been blocked before on performance concerns, so if we were to add this functionality it would have to be in a way where it has absolutely no impact on class construction.

Performance wise I don't see that anything should make it worse than subclassing, particularly given that the function passed would essentially be almost the same thing as super() but without the special magic of setting this locally. (EDIT, Actually in fact I think it would literally just be a wrapper for Reflect.construct(ClassName, args), as it wouldn't be setting new.target unlike super().)

2. I'm not sure if there are many use cases for this type of behavior. I can understand wanting to avoid additional links in the prototype, but it seems like these use cases such as the @validateParams decorator you have here could be potentially solved better by parameter decorators, which we want to add in a future proposal

I agree validateParams specifically would be better with param decorators, but things that manipulate or observe the return value such as memoization, logging, performance marking, etc etc are just rather bizarre with the subclassing pattern because as mentioned above it exposes the subclasses to external code.

3. This increases the scope of an already very large proposal. In fact, it has been brought up multiple times in committee that the current decorators proposal could potentially be broken down into smaller proposals for each type of decorator, in order to speed up the process. The only reason we haven't done this is because we are trying to make sure that the majority of existing use cases in the ecosystem covered, so that we don't introduce a feature that is difficult or impossible to transition to for users of the existing decorators implementations. This is also why the accessor keyword is still part of this proposal, for instance.

I think it's fine to delegate things like this to future proposals, although I think it is important to consider how these sort've things would work, especially in regards to integration with the current proposal. What I had above is fairly vague about integration with various parts of the decorator proposal, particularly such as how it would interact with something like the metadata API. By considering these sort've things now it can be more easily considered as to how it would combine into the existing proposal or if there are things that need to be changed about the current proposal for this and/or other future proposals.

Having said that I don't think there's anything particular in the current design that would block this sort've feature being added in future, but it's possible I've missed something about how any of the features work.

@pzuraq
Copy link
Collaborator

pzuraq commented Aug 2, 2021

By considering these sort've things now it can be more easily considered as to how it would combine into the existing proposal or if there are things that need to be changed about the current proposal for this and/or other future proposals.

Absolutely agree there, and we've been very conscious about leaving space for possible extensions, both syntactically and functionally. If you see any issues with the current proposal that could prevent constructor decorators in the future, please add them here and we can work through them1

@pzuraq
Copy link
Collaborator

pzuraq commented Mar 27, 2022

Closing this as a duplicate of #48

@pzuraq pzuraq closed this as completed Mar 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants