feat(task): improve type checking for tuples#4836
Conversation
|
Thanks Max! I'd like to see a type-only test or two to make sure that this is doing what we expect. I tried it locally and it doesn't seem to infer the args array as const automatically for me: test('type-only args type test', () => {
const accept = <T>(x: T) => x;
class TestElement extends ReactiveElement {
foo: string = 'foo';
bar: number = 123;
task = new Task(this, {
task: ([foo, bar]) => {
accept<string>(foo); // Argument of type 'string | number' is not assignable to parameter of type 'string'.
accept<number>(bar); // Argument of type 'string | number' is not assignable to parameter of type 'number'.
},
args: () => [this.foo, this.bar],
});
}
TestElement;
}); |
|
The 1st overload is typed correctly after the changes in this PR without the need for task1 = new Task(
this,
([string, number]) => {
expectType<string>(string);
expectType<number>(number);
},
() => [this.string, this.number]
);The 2nd overload is not inferred correctly. Appears to be a TypeScript limitation. I also tried the following alternatives, which were not inferred correctly either: // Defining constructors with generic type parameters as per https://github.com/microsoft/TypeScript/issues/10860
export type ITask = {
new <
const T extends ReadonlyArray<unknown> = ReadonlyArray<unknown>,
const R = unknown,
>(
host: ReactiveControllerHost,
task: TaskFunction<T, R>,
args?: ArgsFunction<T>
): Task<T, R>;
new <
const T extends ReadonlyArray<unknown> = ReadonlyArray<unknown>,
const R = unknown,
>(
host: ReactiveControllerHost,
task: TaskConfig<T, R>
): Task<T, R>;
// ... other members omitted
};
///...
const T = Task as unknown as ITask;
task = new T(this, {
task: ([string, number]) => {
expectType<string>(string); // Argument of type 'string | number' is not assignable to parameter of type 'string'.
expectType<number>(number); // Argument of type 'string | number' is not assignable to parameter of type 'number'.
},
args: () => [this.string, this.number],
});// Defining a wrapper function
export function useTask<const T extends ReadonlyArray<unknown>, const R>(
host: ReactiveControllerHost,
task: TaskFunction<T, R>,
args?: ArgsFunction<T>
): Task<T, R>;
export function useTask<const T extends ReadonlyArray<unknown>, const R>(
host: ReactiveControllerHost,
config: TaskConfig<T, R>
): Task<T, R>;
export function useTask<const T extends ReadonlyArray<unknown>, const R>(
host: ReactiveControllerHost,
task: TaskFunction<T, R> | TaskConfig<T, R>,
args?: ArgsFunction<T>
): Task<T, R> {
return new Task(host, task as TaskFunction<T, R>, args);
}
// ...
task3 = useTask(this, {
task: ([string, number]) => {
expectType<string>(string); // Argument of type 'string | number' is not assignable to parameter of type 'string'.
expectType<number>(number); // Argument of type 'string | number' is not assignable to parameter of type 'number'.
},
args: () => [this.string, this.number],
});However, as soon as I disable the 1st function overload, the 2nd one begins getting inferred correctly: constructor(host: ReactiveControllerHost, task: TaskConfig<T, R>) {
// ...
task = new Task(this, {
task: ([string, number]) => {
expectType<string>(string); // all good!
expectType<number>(number);
},
args: () => [this.string, this.number],
});(the useTask wrapper is also inferred correctly once I remove the 1st function overload) We can open an issue in the TypeScript repository for improved Until then, I will add a type test for the 1st function overload - I imagine that's the more commonly used signature? |
🦋 Changeset detectedLatest commit: 17154f7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Maybe my machine is crazy, but I tried reversing the constructor overload order and it seems to correctly infer the types for both forms?? Our docs on lit.dev do prefer the object form for the second arg so it would be good for it to work there. |
fce022d to
2336117
Compare
|
Hi |
Fixes #4829