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

Improve TypeScript types by using variadic tuple instead of overloads #9

Merged
merged 4 commits into from
May 31, 2021

Conversation

privatenumber
Copy link
Contributor

@privatenumber privatenumber commented May 30, 2021

Problem

  • Function signature was using overloads to hardcode support for up to 10 cases

Changes

References

index.d.ts Outdated Show resolved Hide resolved
@privatenumber privatenumber marked this pull request as ready for review May 30, 2021 09:44
@privatenumber
Copy link
Contributor Author

Looks like tsd was outdated and didn't support the variadic tuples syntax. Works fine now.

@privatenumber
Copy link
Contributor Author

Looks like one of tsd@0.11.0's dependencies was previously satisfying a typescript dependency required by @typescript-eslint in xo but is no longer satisfied:

Cannot find module 'typescript'
Error: Failed to load parser '/home/runner/work/p-all/p-all/node_modules/@typescript-eslint/parser/dist/parser.js' declared in 'BaseConfig': Cannot find module 'typescript'
Require stack:
- /home/runner/work/p-all/p-all/node_modules/@typescript-eslint/typescript-estree/dist/parser.js
- /home/runner/work/p-all/p-all/node_modules/@typescript-eslint/parser/dist/parser.js
- /home/runner/work/p-all/p-all/node_modules/eslint/lib/cli-engine/config-array-factory.js
- /home/runner/work/p-all/p-all/node_modules/eslint/lib/cli-engine/cascading-config-array-factory.js
- /home/runner/work/p-all/p-all/node_modules/eslint/lib/cli-engine/cli-engine.js
- /home/runner/work/p-all/p-all/node_modules/eslint/lib/cli-engine/index.js
- /home/runner/work/p-all/p-all/node_modules/eslint/lib/api.js
- /home/runner/work/p-all/p-all/node_modules/xo/index.js
- /home/runner/work/p-all/p-all/node_modules/xo/cli-main.js
- /home/runner/work/p-all/p-all/node_modules/xo/cli.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:885:15)
    at Function.Module._load (internal/modules/cjs/loader.js:730:27)
    at Module.require (internal/modules/cjs/loader.js:957:19)
    at require (internal/modules/cjs/helpers.js:88:18)
    at Object.<anonymous> (/home/runner/work/p-all/p-all/node_modules/@typescript-eslint/typescript-estree/dist/parser.js:20:25)
    at Module._compile (internal/modules/cjs/loader.js:1068:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1097:10)
    at Module.load (internal/modules/cjs/loader.js:933:32)
    at Function.Module._load (internal/modules/cjs/loader.js:774:14)
    at Module.require (internal/modules/cjs/loader.js:957:19)
npm ERR! Test failed.  See above for more details.

Installing the latest typescript as a devDependency yields:

Parsing error: Cannot read property map of undefined
$ npm test

> p-all@3.0.0 test /Users/osame/Documents/public-github/sindresorhus/p-all
> xo && ava && tsd

  index.test-d.ts
  ✖  Parsing error: Cannot read property map of undefined

  index.d.ts
  ✖  Parsing error: Cannot read property map of undefined

  2 errors

Looks like an internal error within xo, perhaps an incompatibility between @typescript-eslint/parser and the latest typescript. There's no expected version of typescript specified in their package.json.

Upgrading xo to the latest yields:

New linting errors
$ npm test

> p-all@3.0.0 test /Users/osame/Documents/public-github/sindresorhus/p-all
> xo && ava && tsd

  index.test-d.ts:2:15
  ✖   2:15   A require() style import is forbidden.                                 @typescript-eslint/no-require-imports
  ✖  16:7    Unsafe assignment of an any value.                                     @typescript-eslint/no-unsafe-assignment
  ✖  18:20   Unsafe member access [0] on an any value.                              @typescript-eslint/no-unsafe-member-access
  ✖  19:20   Unsafe member access [1] on an any value.                              @typescript-eslint/no-unsafe-member-access
  ✖  20:18   Unsafe member access [2] on an any value.                              @typescript-eslint/no-unsafe-member-access
  ✖  21:20   Unsafe member access [3] on an any value.                              @typescript-eslint/no-unsafe-member-access

  index.d.ts:34:15
  ⚠  10:1    Unexpected todo comment: TODO: Refactor the whole definition back....  no-warning-comments
  ✖  34:15   pAll is already defined.                                               @typescript-eslint/no-redeclare
  ✖  37:15   There should be no space after {.                                      @typescript-eslint/object-curly-spacing
  ✖  37:110  There should be no space before }.                                     @typescript-eslint/object-curly-spacing

  index.js:1:1
  ✖   1:1    Do not use "use strict" directive.                                     unicorn/prefer-module
  ✖   2:14   Do not use "require".                                                  unicorn/prefer-module
  ✖   4:1    Do not use "module".                                                   unicorn/prefer-module

  1 warning
  12 errors

I would have to add a tsconfig.json to eliminate the @typescript-eslint/no-unsafe-member-access errors. I can copy the default config that tsd uses, but I know you usually don't have these in your repo. I can disable the prefer-module rules for now.

How would you like me to move forward?

@sindresorhus sindresorhus changed the title refactor(type): use variadic-tuple to remove overloads Improve TypeScript types by using variadic tuple instead of overloads May 31, 2021
@sindresorhus
Copy link
Owner

I'll handle the linting stuff.

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.

2 participants