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

Can't dynamically set private field in initializer #509

Open
mbrowne opened this issue Jun 4, 2023 · 18 comments
Open

Can't dynamically set private field in initializer #509

mbrowne opened this issue Jun 4, 2023 · 18 comments

Comments

@mbrowne
Copy link
Contributor

mbrowne commented Jun 4, 2023

I was trying to upgrade my bound-decorator library (which was based on the Sept. 2018 version of this proposal) to the current proposal, but I ran into an issue with private methods. I'd like to maintain support for private methods as my library currently does, with the goal looking like this:

class ComponentWithPrivateMembers {
    #x = 1

    @bound
    #handleClick() {
        console.log('this.#x = ', this.#x)
    }

    simulateClick() {
        const handler = this.#handleClick
        handler()
    }
}

new ComponentWithPrivateMembers().simulateClick()

The replacement of the method with the bound method needs to happen at initialization time for new instances so that it has access to this, but it seems that context.addInitializer() won't work in this case because there's no way to dynamically set a private field. e.g. this['#x'] is not the same as this.#x.

Am I missing something? If this is indeed impossible with the current class field and decorators specs, is there some future add-on proposal that could address this?

@ljharb
Copy link
Member

ljharb commented Jun 4, 2023

The decorator should be receiving a get and set function that provides you with access to the private field, but that may not work for private methods.

@mbrowne
Copy link
Contributor Author

mbrowne commented Jun 4, 2023

@ljharb Thanks. I just tried it and unfortunately the context.access object only contains get and has methods (not a set method) for methods...

@mhofman
Copy link
Member

mhofman commented Jun 4, 2023

Interesting, private methods are technically installed as instance fields but I don't believe there is any way to redefine them currently, so I suspect some implementations likely share a class shape for all instances. Allowing to replace the value for specific instances would likely introduce implementation complexity. I'm wondering if there should be a way in private methods decorators to flag that the return value is an initializer (like for class fields) instead of a replacement method.

Edit: actually we'd have to change the behavior for all private methods decorators as it's not possible to chain an initializer returning decorator into a method override decorator. That seems suboptimal for the common wrapping use case.

Edit2: thinking about this more there may be a way to combine both without the decorators having to coordinate, but it's a bit complicated (the value would have to be a function that forwards to the current function for the instance in the stack of decorators)

@mbrowne
Copy link
Contributor Author

mbrowne commented Jun 4, 2023

In case it helps, for this use case at least it wouldn't be necessary to replace the method (although maybe that would be most straightforward)...turning it into a regular private field that happens to have a value of a function would work just fine.

@mbrowne
Copy link
Contributor Author

mbrowne commented Jun 4, 2023

Of course, if you want to use private fields and methods, there's always this workaround—technically a private field (arrow function) rather than a private method:

class ComponentWithPrivateMembers {
    #x = 1

    #handleClick = () => {
        console.log('this.#x =', this.#x)
    }

    simulateClick() {
        const handler = this.#handleClick
        handler()
    }
}

...which doesn't suffer from the same downsides as public properties for bound methods (related to the prototype and inheritance, which are irrelevant for private methods). But ideally it would be nice to have consistent syntax, with a @bound director that you could use on both public and private methods.

@mhofman
Copy link
Member

mhofman commented Jun 4, 2023

Here is another thought. Since decorators are now structured to never change the type of the thing it decorates, I'm wondering if allowing the accessor modifier on private methods would make sense? However to be honest, if we're going to require the author to change the syntax of the declaration, maybe asking to declare it as a private field containing a function in the first place may not be too much of a stretch.

@mbrowne
Copy link
Contributor Author

mbrowne commented Jun 21, 2023

@mhofman

I'm wondering if allowing the accessor modifier on private methods would make sense?

I think it could be a bit weird to expand the concept of "accessor" to include accessing a field or method internally that you can already access...what would it actually mean to have such an accessor?

Do you think this option is still worth exploring:

Edit2: thinking about this more there may be a way to combine both without the decorators having to coordinate, but it's a bit complicated (the value would have to be a function that forwards to the current function for the instance in the stack of decorators)

@biro456
Copy link

biro456 commented Nov 12, 2023

@mbrowne this is the workaround I came up with to make @bound work with private fields:

Actually only works for singletons 🤦
export function bound<T extends object, V extends (this: T, ...args: any[]) => any>(target: V, context: ClassMethodDecoratorContext<T, V>): V {
    let instance: T;

    context.addInitializer(function () {
  	  instance = this;
    });

    return function (this: T, ...args: Parameters<V>): ReturnType<V> {
  	  return target.call(instance, ...args);
    };
}

It's not exactly the behaviour one would expect, but it gets the job done.

@mbrowne
Copy link
Contributor Author

mbrowne commented Nov 12, 2023

@biro456 Thanks, it's nice to know this is possible. I'm not sure if the this: T part in the function you're returning is necessary unless you found that something didn't type-check correctly without it. I was just testing in JS (not TS) but this worked for me (I also changed .call to .apply but that doesn't really matter):

    return (...args) => {
        return target.apply(instance, args)
    }

@biro456
Copy link

biro456 commented Nov 13, 2023

And a day later I realize it only works for singletons. 🤦

@mbrowne
Copy link
Contributor Author

mbrowne commented Nov 13, 2023

Aww ok, I hadn't inspected it closely yet but that makes sense. I believe the bound function only runs once.

@anlexN
Copy link

anlexN commented Nov 14, 2023

WHEN is proposal-decorators final?

@ljharb
Copy link
Member

ljharb commented Nov 14, 2023

It's already stage 3, so unless implementers discover a previously unknown problem while implementing it, it's pretty much as final as it'll ever be.

@anlexN
Copy link

anlexN commented Nov 15, 2023

It's already stage 3, so unless implementers discover a previously unknown problem while implementing it, it's pretty much as final as it'll ever be.

so it is almost final, not true final. do it have roadmap?

@ljharb
Copy link
Member

ljharb commented Nov 15, 2023

There is never a “true final” for anything.

@nicolo-ribaudo
Copy link
Member

@anlexN Browsers are implementing this proposal right now. Once two of them ship it enabled by default, the champion will propose advancing this proposal to Stage 4 (the final stage) and it will be merged to the main specification.

@pzuraq
Copy link
Collaborator

pzuraq commented Feb 2, 2024

So this was discussed previously, it was shot down due to performance concerns around being able to redefine private class methods. It's something that could be considered in the future, definitely, as it wouldn't be a breaking change to add the access.set method to context I don't think.

@mantou132
Copy link

mantou132 commented Jul 13, 2024

I really want this feature, which can make getter memorize:

class A {
  // getter will be re-calculated when dependent update
  @memo(() => [dep1, dep2])
  get #data() {
    // slow calculate
    return performance.now();
  }
}

The current public field work is very good, I hope to use a private getter

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

8 participants