Skip to content

feat(task): improve type checking for tuples#4836

Merged
augustjk merged 5 commits intolit:mainfrom
maxpatiiuk:max/4829
Nov 26, 2024
Merged

feat(task): improve type checking for tuples#4836
augustjk merged 5 commits intolit:mainfrom
maxpatiiuk:max/4829

Conversation

@maxpatiiuk
Copy link
Contributor

Fixes #4829

@justinfagnani
Copy link
Collaborator

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;
  });

@maxpatiiuk
Copy link
Contributor Author

maxpatiiuk commented Nov 17, 2024

The 1st overload is typed correctly after the changes in this PR without the need for "as const":

      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 const inference in function overloads, though I imagine it is complicated to make that work in a performant way.

Until then, I will add a type test for the 1st function overload - I imagine that's the more commonly used signature?

@changeset-bot
Copy link

changeset-bot bot commented Nov 17, 2024

🦋 Changeset detected

Latest commit: 17154f7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@lit/task Patch

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

@augustjk
Copy link
Member

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.

@augustjk augustjk force-pushed the max/4829 branch 2 times, most recently from fce022d to 2336117 Compare November 22, 2024 09:01
Copy link
Collaborator

@justinfagnani justinfagnani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@augustjk augustjk merged commit 05691ba into lit:main Nov 26, 2024
@maxpatiiuk maxpatiiuk deleted the max/4829 branch December 27, 2024 19:54
@maxpatiiuk
Copy link
Contributor Author

Hi
Thanks for merging this PR
Could you publish a new @lit/task release that includes this fix?

@lit-robot lit-robot mentioned this pull request Jan 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Lit/Task]: Improve type-checking by using const

3 participants