Skip to content

'get' and 'set' accessors cannot declare 'this' parameters. #39254

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

Closed
rgbui opened this issue Jun 25, 2020 · 16 comments
Closed

'get' and 'set' accessors cannot declare 'this' parameters. #39254

rgbui opened this issue Jun 25, 2020 · 16 comments
Labels
Question An issue which isn't directly actionable in code

Comments

@rgbui
Copy link

rgbui commented Jun 25, 2020

This error occurs after vSCode is updated.
code example:
views/view.model.ts

export class View$Model {
    get closestModelView(this: View): View {
        return this.closest(x => x.isModel == true || x.isTemplate);
    }
    get closestModel(this: View): Model {
        var view = this.closestModelView;
        if (view) {
            return view.model;
        }
        return null;
    }
}

view.ts

///<reference path='views/view.model.ts'/>
export class View extends ViewParse.Plug.BaseControl{  ...  }
export interface View { 
     on(name:'click',fn:(view:View)=>void)
     ....
}
export interface View extends View$Model { }
Util.inherit(View, View$Model)

tsconfig.json

{
    "compilerOptions": {
        "outFile": "../ts/view.js",
        "declaration": true,
        "target": "es6",
        "removeComments": true,
        "sourceMap": true
    }
}

current typescript version:3.9.5
current @types/node version:14.0.14
current vscode version:1.45.1
image

the "this: View" Will report an error, prompt: 'get' and 'set' accessors cannot declare 'this' parameters.
I know what this error message means, but it's always been written like this.
Has Typescript tweaked its rules?
I don't want to change the code. I don't like code that is too long for a single file, and I have a lot of code to change.This pattern simulates partial classes.
I want to know what to do about it, the reduced version, the special quote statement, or what?
I have checked the sample question, but no one mentioned it。
This happens every time you compile, which is bad. To seek help

@WhileTrueEndWhile
Copy link

A 'get' accessor cannot have parameters. (1054)

How should another this then be bound to a property at all?

@rgbui
Copy link
Author

rgbui commented Jun 25, 2020

export interface View extends View$Model { }
The get property and the method inside the View$Model declare "this:View"
This has been used in previous versions, but the recent update of VSCode has caused this problem.

the View class inherits View$Model, so View$Model actually can't pull the View Class, but at this point the View declares export Interface which is global, so there's nothing wrong with declare this:View in View$Model.In fact, the method inside the View$Model used this:View without any problems, just 'get' and 'set' accessors report prompt: 'get' and 'set' accessors cannot declare 'this' parameters.

@WhileTrueEndWhile
Copy link

OK, I think it can be done: https://stackoverflow.com/questions/37973290/javascript-bind-method-does-not-work-on-getter-property#answer-37973399
I'm not involved here, but I think a playground link might be helpful.

@RyanCavanaugh RyanCavanaugh added the Question An issue which isn't directly actionable in code label Jun 25, 2020
@RyanCavanaugh
Copy link
Member

This was incorrectly allowed and never actually worked. See #36883

@rgbui
Copy link
Author

rgbui commented Jun 26, 2020

Thank you for your help, @WhileTrueEndWhile@RyanCavanaugh
I see the problem, but my one is different from this one。
I declare the inheritance of the class
export interface View extends View$Model { }
That alone, of course, would not work in practice in JavaScript
So let's add a little bit of code like this
Util.inherit(View, View$Model)
The source code for this method looks like this

   inherit(Mix, ...mixins) {
            return using.classExtend.apply(using, arguments);
        },
        classExtend(Mix, ...mixins) {
            function copyProperties(target, source) {
                for (let key of Reflect.ownKeys(source)) {
                    if (key !== "constructor"
                        && key !== "prototype"
                        && key !== "name"
                    ) {
                        let desc = Object.getOwnPropertyDescriptor(source, key);
                        Object.defineProperty(target, key, desc);
                    }
                }
            }
            for (let mixin of mixins) {
                copyProperties(Mix, mixin);
                copyProperties(Mix.prototype, mixin.prototype);
            }
            return Mix;
        }

It's been written like this for a couple of years, and it works without any problems,
It was the latest update to VSCode that reported these errors

The "this" statement in the parameter should check inheritance, but I declare inheritance。The #36883 doesn't。
I think people who purposely write 'this' know exactly what they're doing. 36883 people don't understand Typescript and JS very well, but get it wrong.

I'm going to simulate partial classes.Write a class as multiple files.Because the code is too long for a single file, it's not particularly maintainable.
If not, how do partial classes express themselves?
@RyanCavanaugh How do you do this if you emulate partial classes, current Typescript versions?
In the current version, the "get" and "Set" Accessors "simulation doesn't seem to work.

@typescript-bot
Copy link
Collaborator

This issue has been marked as 'Question' and has seen no recent activity. It has been automatically closed for house-keeping purposes. If you're still waiting on a response, questions are usually better suited to stackoverflow.

@kobiburnley
Copy link

kobiburnley commented Jul 24, 2020

This change also broke something that I sometimes do with TypesScript & JS prototype - something like "extension methods"
Playground

declare global {
  interface Array<T> extends MyArrayExtension<T> {}
}

@extention(Array)
export abstract class MyArrayExtension<T> {
  get myLength(this: Array<T>) {
    return this.length;
  }
}

function extention(clz: any) {
  return function (target: any) {
    // iterate 'target' methods, getters, setters - and add them to 'clz.prototype'
  }
}

@tadhgmister
Copy link

@rgbui

I see the problem, but my one is different from this one。

Not it isn't, nothing is stopping a user from doing new View$Model().closestModelView having it type check and getting a runtime error. @kobiburnley is a lot safer by labeling the class as abstract so someone would need to subclass it then instantiate which is much less likely to do accidentally, but still the point is that typescript can't validate the this parameter on accessors in the same way as methods so letting you do it leads to a false sense of security.

I think people who purposely write 'this' know exactly what they're doing.

You could make this argument for pretty much everything, if we just assume the user knows what they are doing at all times then why typescript at all? You know this isn't the case because your next statement contradicts it:

36883 people don't understand Typescript and JS very well, but get it wrong.

As the person who raised that ticket I do take personal offence to this, I very clearly outline my actual use case in my second comment, I was very well aware that I was using this correctly and the runtime behaviour was correct.

I'm going to simulate partial classes.Write a class as multiple files.Because the code is too long for a single file, it's not particularly maintainable.
If not, how do partial classes express themselves?
@RyanCavanaugh How do you do this if you emulate partial classes, current Typescript versions?
In the current version, the "get" and "Set" Accessors "simulation doesn't seem to work.

Normally when there are definitions in your class that are implemented in a way you can't express to typescript, you create an interface to merge with the class, in your case this would cause cyclical references so to break it I would recommend Picking the properties that you actually use, something like this:

// pick only the fields of View that this extension relies on.
export interface View$Model extends Pick<View, "closest"> {}
export abstract class View$Model { // added abstract to ensure people don't instantiate this directly by mistake.
    get closestModelView(): View {
        return this.closest(x => x.isModel == true || x.isTemplate);
    }
    get closestModel(): Model | null {
        const view = this.closestModelView;
        if (view) {
            return view.model;
        }
        return null;
    }
}

This has an added benefit that you end up documenting the dependencies between your partial class parts at the cost that intellisense won't suggest fields that aren't already mentioned in the Pick.

Another option that may be more applicable for @kobiburnley's case is to just use good old fashioned this as Type if there are relatively few places it is needed:

@extension(Array)
export abstract class MyArrayExtension<T> {
    get myLength() {
        return (this as unknown as Array<T>).length;
    }
}

This has the benefit that if something does go wrong the as unknown as Type sticks out as something that is probably the root issue at the cost of being noisy.

@anatoliyarkhipov
Copy link

Considering this restriction, is that possible to implement a Singleton mixin with a static getter? 🤔

For example, I can make smth like this with a static method:

type Constructor = new (...args: any[]) => {}
function Singleton<TBase extends Constructor>(Base: TBase) {
  return class Singleton extends Base {
    static _i: any
    static i<T>(this: (new (...args: any[]) => T)): T {
      return (this._i = this._i || new this);
    }
  }
}

// - - -

class MyClass extends Singleton() {
  hello() {
    // ...
  }
}

MyClass.i().hello()

But is there a way to achieve a MyClass.i.hello() interface? When I try to define the i as a getter, I get the "cannot declare this" error.

@tadhgmister
Copy link

@anatoliyarkhipov this is nearly identical to what I was trying to accomplish in my original use case, the answer is unfortunately no. Having a this parameter implicitly makes the function generic and Typescript can't attach generics on properties. (since getters are the only case where this would make sense)

I believe my exact words were:

Right away typescript told me that generics on a getter is not valid, but it took me a while to find that it also doesn't support a this parameter correctly.

It would be amazing if typescript was capable of representing that HookCls.JSX has a different type on subclasses, (and it can be transformed based on a generic on the getter) but I recognize how much changes it would take to get something like this to be supported for such a very niche use case.

I'm not saying it would be fundementally impossible to support, just that the effort to get getters to work seemlessly as properties as it currently does but also be treated as potentially generic functions would be substantial. If the use cases for this kind of design increases maybe it is worth visiting but for now I think it still falls under the "too niche to be worth implementation effort" category of features.

@anatoliyarkhipov
Copy link

anatoliyarkhipov commented Jan 18, 2021

@tadhgmister Okay, fair enough. I didn't intend to argue, I just wasn't sure if I got it right.

Also, one workaround I found, and which I'd call acceptable (to my taste), is to declare the static property on the heir. It feels almost like a configuration property required by the "API" of Singleton.

class MyClass extends Singleton() {
  static i: MyClass

  hello() {
    // ...
  }
}

MyClass.i.hello

@jpilkahn
Copy link

jpilkahn commented Mar 28, 2021

Considering this restriction, is that possible to implement a Singleton mixin with a static getter?

But is there a way to achieve a MyClass.i.hello() interface? When I try to define the i as a getter, I get the "cannot declare this" error.

@anatoliyarkhipov this is nearly identical to what I was trying to accomplish in my original use case, the answer is unfortunately no.

Respectfully, @tadhgmister, I think that's incorrect.

  • Static members cannot reference type parameters, not contesting that.
  • 'get' and 'set' accessors cannot declare 'this' parameters, not contesting the original issue either.
  • But, it is possible to "implement a Singleton mixin with a static getter" as in the most recently asked question, as mixins (already employed in the attempt in the question) facilitate generic static methods and a modified this context isn't required.
type Ctor<T = {}> = new (...args: any[]) => T

function Singleton<T extends Ctor>(Base: T) {
    return class Singleton extends Base {
        private static readonly _instance: T
       
        static get instance(): T {
              return (this._instance = this._instance || new Base())
        }
    }
}

class MyClass extends Singleton() {
    hello() {}
}

MyClass.instance.hello()

Having pointed out that it's possible, @anatoliyarkhipov, personally, I fail to understand why you'd want to do that in the first place. As far as I see it, that'd be quite the anti-pattern.

Creating generic static properties is definitely a fair use case for mixins, generally. What I don't see at all, is the point of a "Singleton" in JS / TS. The language is full of them already. They're just usually not explicitly called that.

  • Classes declared with the class keyword are singletons, which is incidentally the reason why static properties cannot be made generic (at compile time anyway, and the runtime support of mixins is needed).
  • Modules (files containing one or more export or import keywords) are singletons, which is why you can use the same class, constant, function and variable names in multiple modules and even export the same name multiple times. Name clashes only happen, when attempting to import two named exports from different modules into the same consuming module.
  • Closures ...
  • Namespaces ... (you catch my drift)

Idiomatic Singleton

// begin of file
export class MyClass {
    private constructor() {}
    static hello() {}
}
// end of file
// other file
import { MyClass } from '../path/to/modules-are-singletons'

MyClass.hello()

or even bolder architecture:

// begin of file
const property = 'I am a private, static property of a singleton'
export function hello() {
    console.log(property)
}
// end of file
// other file
import { hello } from '../path/to/modules-are-singletons'

hello()

Why so complicated?

@anatoliyarkhipov
Copy link

@jpilkahn Unfortunately, your example doesn't achieve what I wanted to achieve, unless I'm thoroughly mistaken. What I want for the static getter, is to return an instance of MyClass, while in your example it's an instance of the Base class, that can be passed as the argument for Singleton (and in your example you're not actually passing it, so I suspect it will throw on || new Base).

image

As for why I want to do it this way... well...

image

@tadhgmister
Copy link

tadhgmister commented Apr 12, 2021

But, it is possible to "implement a Singleton mixin with a static getter" as in the most recently asked question, as mixins (already employed in the attempt in the question) facilitate generic static methods and a modified this context isn't required.

I'm sorry but your Singleton example does not work at runtime. Also there are a few issues with the generics, specifically Base and self._instance are the same type and it isn't flagged as an issue in non strict settings because the default generic value of {} has incredibly misleading behaviour. But I want to look at runtime behaviour only for a moment.

the statement new Base() calls the argument passed to Singleton so the idea of class MyClass extends Singleton() will try to instantiate undefined and fail.

If you had a base class then it would instantiate the base class, not the subclass that is desired:

class A {
    hello = "world"
}
class B extends Singleton(A) {
    foo = "bar"
}
console.log(B.instance.hello) // works because it is in the base passed to Singleton
console.log(B.instance.foo) // undefined because you didn't instantiate B, you instantiated A

The only way around this is to instead use new this() inside the static method, this will instantiate the referred class, I.E. B.instance will have this == B in the getter. This works totally fine for methods as is shown above but there isn't a way for typescript to deal with it for getters.


That said I totally agree with the idea that needing a dynamic getter is kind of overkill, just doing class InternalSingletonClass then export const MySingleton = new InternalSingletonClass() and if wanting to lazy load stuff in the constructor is a concern find a better way to optimize that, but anyway.

@sharshuv-quotient
Copy link

Is it planned for typescript to add support for that? Allowing this could be really useful

@tadhgmister
Copy link

Is it planned for typescript to add support for that? Allowing this could be really useful

not planned unless you can find an issue specifically requesting it, this issue and the root #36883 are both just about whether it is allowed syntactically - no one's opened the discussion for proper support. (as far as I know)

If you do want to make a case for support raise a new issue (search for dups first, I haven't) and try to make an argument for use cases that would benefit, probably link to mine here #36883 (comment), as I say there it feels like a niche use-case but maybe there is more substance than I realize.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Question An issue which isn't directly actionable in code
Projects
None yet
Development

No branches or pull requests

9 participants