Closed karol-majewski closed 5 years ago
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
.
…
parse
is not called withthis
ofJSON
.
You're absolutely right, the correct way to call it would be without point-free style, e.g. arg => JSON.parse(arg)
.
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
Nice! Let's do that
@karol-majewski I just realised, your example of JSON.parse
above doesn't work with the default types of JSON.parse
😢
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);
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)
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: https://github.com/typescript-eslint/typescript-eslint/issues/1112.
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 anythis
:
In this specific example, yes, but it's easy to find an example where it doesn't:
const { getItem } = localStorage;
getItem('foo'); // Runtime exception
I'm sure they had good reasons to do it this way, though.
Let's find out: https://github.com/microsoft/TypeScript/issues/36100 🍿
Sounds like they don't do it for performance reasons https://github.com/microsoft/TypeScript/issues/35451
Off the back of the discussion in https://github.com/typescript-eslint/typescript-eslint/pull/1279, I'm convinced this should be left to a lint rule.
For this to work:
this: this
andpipe
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.
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.
Let's suppose I have a function that uses
this
other thanWindow
. In this example, I have a static method that must be called withthis
ofJSON
.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.