Compile-time errors related to this are gone

[karol-majewski]

Let's suppose I have a function that uses this other than Window. In this example, I have a static method that must be called with this of JSON.

declare class JSON {
  /**
   * `parse` should always be called with `this === JSON`.
   */
  static parse(this: JSON, input: string): object;
}

const { parse } = JSON;

JSON.parse('1'); // Good
parse('1') // Bad, we get a compile-time error as expected

TypeScript will warn you when you try and call parse as if it were a standalone function.

However, when we do the same thing with pipe/pipeWith, the error is gone.

pipeWith(
  'foo',
  JSON.parse // Good: `parse` is called with `this` of JSON
)

pipeWith(
  'foo',
  parse // Bad: this should give us a compile-time error!
)

[OliverJAsh]

Good catch. Is there anything we can do to prevent these mistakes? My guess is there's not, since the function is just being passed as a value—TypeScript doesn't know how/when pipeWith will use that function. IIRC there are lint rules to help catch mistakes like this.

P.S. in your first example…

pipeWith(
  'foo',
  JSON.parse
)

… parse is not called with this of JSON.

[karol-majewski]

… parse is not called with this of JSON.

You're absolutely right, the correct way to call it would be without point-free style, e.g. arg => JSON.parse(arg).

[karol-majewski]

If functions dependent on this cannot be used with pipe, we can reject them in compile-time:

function pipeWith<A, B>(a: A, ab: (this: void, a: A) => B): B

This makes it type-safe.

JSON.parse('1');                         // ✅ Works
parse('1');                              // ✅ Compile-time error
            
pipeWith('foo', JSON.parse);             // ✅ Compile-time error
pipeWith('foo', parse);                  // ✅ Compile-time error
pipeWith('foo', arg => JSON.parse(arg)); // ✅ Works

TypeScript Playground

[OliverJAsh]

Nice! Let's do that

[OliverJAsh]

@karol-majewski I just realised, your example of JSON.parse above doesn't work with the default types of JSON.parse 😢

[OliverJAsh]

In hindsight, isn't this better handled with something like https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/unbound-method.md?

IIUC, the benefit the lint rule has over the changes made by this PR is that it will catch mistakes for methods where this is not explicitly defined, e.g. class methods:

class Foo {
  bar(key: string) {
    return key;
  }
}

const foo = new Foo();
// No type error here ❌ 
// ESLint error here ✅ 
const result = pipeWith('foo', foo.bar);

[karol-majewski]

That's right — the snippet in the original post was using a different definition.

A version that is closer to how global constructors are defined.

Not everyone is using ESLint. It would be best if such an error came from the right place — the type checker itself. Perhaps it's worth seeing if the standard library types can be changed in a way that requires a specific this.

 interface JSON {
-    parse(text: string, reviver?: (this: any, key: string, value: any) => any): any;
+    parse(this: JSON, text: string, reviver?: (this: any, key: string, value: any) => any): any;
 }

Have you tried using this rule? I tried using its TSLint cousin and it didn't cover all cases. These two didn't raise an error (they should):

parse('1'); // No error (expected one)                   
pipeWith('foo', parse); // No error (expected one)                   

[OliverJAsh]

Perhaps it's worth seeing if the standard library types can be changed in a way that requires a specific this.

That could help, although note that in the specific example of JSON.parse, it does work with any this:

const { parse } = JSON;
parse('{ "foo": 1 }') // => { foo: 1 }

This makes me wonder why TypeScript doesn't prescribe a this type automatically for class methods, e.g.

class Foo {
  value = 2
  add(number: number) {
    return this.value + number;
  }
}

const foo = new Foo();
// No type error here because `foo.add` doesn't have an *explicit*` this type ❌ 
const result = pipeWith(1, foo.add);

It would be very annoying to manually type this every time, for every method on every class!

-  add(number: number) {
+  add(this: Foo, number: number) {

IMO this shouldn't be necessary because TypeScript clearly knows the type of this already as we're using it inside the method implementation. I wonder if this has been considered by the TS team?

Unless consumers are very careful to manually + explicitly annotate the this type, they won't get any type errors when trying to use pipe/pipeWith.

Have you tried using this rule? I tried using its TSLint cousin and it didn't cover all cases. These two didn't raise an error (they should):

I used the TSLint rule a long time ago and it seemed to be useful. Yesterday I played with the ESLint variant here: https://github.com/unsplash/unsplash-web/pull/4467. I think the ESLint rule has the same problem though: typescript-eslint/typescript-eslint#1112.

[karol-majewski]

IMO this shouldn't be necessary because TypeScript clearly knows the type of this already as we're using it inside the method implementation. I wonder if this has been considered by the TS team?

Exactly — it would be safe to implicitly apply a this equal to the class instance for non-static methods. As far as I know, this in methods is used by the community only to achieve f-bounded polymorphism, which is a fancy name for telling when a method can be called based on the provided type argument.

interface Collection<T> {
  // This methods is available for any T.
  toArray(): T[]; 

  // This method is only available for array types, where T is a number
  sum(this: Collection<number>): number;
}

declare const strings: Collection<string>
declare const numbers: Collection<number>

strings.sum(); // Compile-time error
numbers.sum(); // OK

But since Collection<number> is covariant in relation to T, I don't see why applying an implicit this better than any would cause problems. I'm sure they had good reasons to do it this way, though.

That could help, although note that in the specific example of JSON.parse, it does work with any this:

In this specific example, yes, but it's easy to find an example where it doesn't:

const { getItem } = localStorage;

getItem('foo'); // Runtime exception

[OliverJAsh]

I'm sure they had good reasons to do it this way, though.

Let's find out: microsoft/TypeScript#36100 🍿

[OliverJAsh]

Sounds like they don't do it for performance reasons microsoft/TypeScript#35451

Off the back of the discussion in typescript-eslint/typescript-eslint#1279, I'm convinced this should be left to a lint rule.

For this to work:

1. all class methods must have this: this and
2. all higher-order functions (not just pipe and pipeWith but everything else such as addEventListener) that may receive class methods as a parameter must have this: void.

It doesn't hurt to keep this: void in pipe and pipeWith, but we're never going to really benefit from it, given that nearly the class methods we use will not carry their this type. We can fix that where we define the types, but that would rely on a lot of discipline, and a lot of the time it's outside of our control (i.e. third party types). For that reason, perhaps we should revert this change and use lint rules instead, which will not only address this use case (pipe and pipeWith) but also all other usages of higher-order functions.

[karol-majewski]

I don't think there's any harm in keeping it for the benefit of users who do make the effort and type their methods deliberately?

Example: the Snowplow collector requires a this of TrackersByNamespace.

declare const trackers: Snowplow.TrackersByNamespace;

declare global {
  interface Window {
    snowplow: {
      (callback: (this: TrackersByNamespace) => void): void;
    }
  }
}

pipeWith(
  trackers,
  window.snowplow,
);

If we lift that requirement from pipeWith, the type error will be gone.