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

Tighten up multiArgs TypeScript type constraints #22

Merged
merged 2 commits into from
Mar 13, 2019

Conversation

AaronFriel
Copy link
Contributor

This PR modifies the type constraints by using rest elements/spread types by default, and treating the single-argument (non-multiArgs) case as special by passing it to Emitter as [EmittedType] (note the brackets, making it a single element tuple).

This provides better ergonomics on iterator, e.g.: this case with Express:

      const testRouteIterator = pEvent.iterator<[Request, Response]>(app as any, 'test', {
        multiArgs: true,
      });

      for await (const callback of testRouteIterator) { // callback: [Request, Response]
        const [req, res]: [Request, Response] = callback;
      }

image

Comparison

Relative to what version 3.0 emits for types:

Using iterator<[Request, Response]>

      const testRouteIterator = pEvent.iterator<[Request, Response]>(app as any, 'test', {
        multiArgs: true,
      });

      for await (const callback of testRouteIterator) { // callback: [Request, Response][]
        const [req, res]: [Request, Response] = callback;
      }

image

This doesn't make sense, because it's not an array of [Request, Response] tuples.

Using iterator<Request, [Response]>

      const testRouteIterator = pEvent.iterator<Request, [Response]>(app as any, 'test', {
        multiArgs: true,
      });

      for await (const callback of testRouteIterator) { // callback: (Request | [Response])[]
        const [req, res]: [Request, Response] = callback;
      }

image

This doesn't make sense because the first argument is never a Response, and the second argument is never a Request, but indeed, this type checks, but it cannot possibly be true:

      for await (const callback of testRouteIterator) { // callback: (Request | [Response])[]
        const arg0: Request | [Response] = callback[0];
        const arg1: Request | [Response] = callback[1];
        const arg2: Request | [Response] = callback[2];
        const arg3: Request | [Response] = callback[3];
        const arg4: Request | [Response] = callback[4];
      }

@sindresorhus
Copy link
Owner

// @BendingBender

index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
@BendingBender
Copy link
Contributor

I think this is a breaking change...

@AaronFriel
Copy link
Contributor Author

AaronFriel commented Mar 13, 2019

@BendingBender Correct, I don't usually bump version numbers as part of PRs but if I had, this would suggest a bump to 4.0.0.

@sindresorhus sindresorhus changed the title Tighten up multiArgs type constraints. Tighten up multiArgs TypeScript type constraints Mar 13, 2019
@sindresorhus sindresorhus merged commit c8bddfa into sindresorhus:master Mar 13, 2019
@AaronFriel
Copy link
Contributor Author

@sindresorhus @BendingBender I just want to say thank you for your speedy reviews and approval, I know it is too often a thankless job. :)

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.

4 participants