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

Suggestion when non-bound or non-arrow function is passed to another function. #31684

Closed
5 tasks done
trusktr opened this issue May 30, 2019 · 13 comments
Closed
5 tasks done
Labels
Duplicate An existing issue was already created

Comments

@trusktr
Copy link

trusktr commented May 30, 2019

Search Terms

related #10285

#15

Suggestion

When we pass a function to external code in our class, f.e.

foo.on('click', this.handleClick)

it may be nice for TypeScript to provide a warning or error if the function being passed is not bound to this or is not an arrow function closing the same this.

I don't see any warning or error, despite the status of #15. Maybe I missed something?

Use Cases

It can prevent runtime errors.

Examples

See above.

Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.
@jcalz
Copy link
Contributor

jcalz commented May 31, 2019

Wouldn't this be a breaking change in existing TypeScript code (unless it's behind an opt-in compiler flag)?

@fatcerberus
Copy link

fatcerberus commented May 31, 2019

From what I understand, the default for functions without an explicit this type is essentially this: any (albeit a “smart” polymorphic any), for backward-compatibility reasons. If you want this check, you have to opt in; for classes this as as simple as method(this: this, ...args).

It might be nice to have a flag to make the behavior stricter, e.g. something like strictThisTypes, but at least for the moment the current behavior is by design.

edit: So this is a way you can get the type checker to help you out here:

class Foo {
    on(event: 'click', callback: (this: void) => void) {
        callback();
    }
}

class Bar {
    handleClick(this: this) {}
    
    blub() {
        const foo = new Foo();
        foo.on('click', this.handleClick);  // error - 'void' is not assignable to 'this'
    }
}

You need to both declare the method as handleClick(this: this) and the callback parameter as callback: (this: void) => void. It would definitely be nice to have a flag that provides this stricter this typing as an opt-in.

@dragomirtitian
Copy link
Contributor

Related to this I believe #7968

@nicojs
Copy link

nicojs commented May 31, 2019

It would be nice to have a more seamless experience. A strict-family compiler flag that automatically declares the this fake parameter for methods of classes would be much appreciated (--strictMethodBinding?). In practice you'll rarely want methods to have a polymorphic any as this, right?

Maybe the compiler could even suggest a fix when used incorrectly (class properties: handleClick = () => { }).

This is a class of errors that is currently uncaught. I see a lot of people struggling with this (no pun intended). It's now even more common with native web components and libraries like lit element

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label May 31, 2019
@RyanCavanaugh
Copy link
Member

We tried having this as a behavior, but the implementation effectively makes every class method in the world generic, and the performance implications were beyond what anyone would consider acceptable. The current state of things under --strict is as much as we think is achievable in the current implementation of the compiler.

@fatcerberus
Copy link

the implementation effectively makes every class method in the world generic

Can you explain what you mean by this? Class methods without a this type are embarassingly generic today, as for the purposes of assignability and callability the default behavior is essentially this: any (with some polymorphic magic to make this act as the proper type inside the method body). Defaulting methods to this: this and free functions to this: void would make things less generic, not more.

Performance implications I understand, but "every method in the world is generic" is the situation we already have today.

@RyanCavanaugh
Copy link
Member

By "generic" I mean "having a type parameter" and thus subject to more complex rules around assignability

@fatcerberus
Copy link

I still don't understand - this: MyClass is no more a "type parameter" than arg1: MyClass - it's just a hidden extra parameter supplied by the semantics of the language rather than explicitly at the call site.

I'm probably missing something really obvious here...

@fatcerberus
Copy link

Unless you mean specifically this: this which I guess is generic because of the polymorphic nature of this as a type... hmm.

@RyanCavanaugh
Copy link
Member

Unless you mean specifically this: this which I guess is generic because of the polymorphic nature of this as a type... hmm.

Bingo

@typescript-bot
Copy link
Collaborator

This issue has been marked as a 'Duplicate' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@nicojs
Copy link

nicojs commented Jun 6, 2019

Would it still be possible to generate this: MyClass with a --strict- compiler option when the class can be named? That would already solve a bunch of issues.

@dragomirtitian
Copy link
Contributor

dragomirtitian commented Jun 6, 2019

You can opt into it using an annotation directly on the method:

class MyClass {
  m(this: this) {

  }
}

function onFoo(cb: (this: void) => void) {
  cb();
}

onFoo(new MyClass().m) //Error 
let o = new MyClass()
onFoo(o.m.bind(o)) // ok an well typed under strictBindCallApply

Not sure how useful this is since you have to do it on the method but though it is worth mentioning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

7 participants