Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Consider making @types/* dev dependencies #1326

Closed
daniel-white opened this issue Oct 24, 2023 · 7 comments · Fixed by #1569
Closed

Consider making @types/* dev dependencies #1326

daniel-white opened this issue Oct 24, 2023 · 7 comments · Fixed by #1569

Comments

@daniel-white
Copy link

It would be great if the @types/* were not direct dependencies but dev dependencies as to avoid the pit fall of having the types for node 20, but running under node 18.

@SBoudrias
Copy link
Owner

SBoudrias commented Oct 26, 2023

This cannot work today because Inquirer code accesses the node types. So it cannot ship without the @types/node as the imported types will be missing. Specifically this caused a bug in the past, see #1241

I'm open to review this or change the code somewhat to get to a better result. But I'll need documentation and reference from you supporting how this is possible. We can work together on that if you're willing to.

@daniel-white
Copy link
Author

it looks like you only ship javascript in your package. so the types package is not directly needed. i would advise using this as a devDependency https://stackoverflow.com/questions/18875674/whats-the-difference-between-dependencies-devdependencies-and-peerdependencie

@SBoudrias
Copy link
Owner

That is not the case, the types are shipped. See https://www.npmjs.com/package/@inquirer/core?activeTab=code under dist/esm/types/ for one example.

@daniel-white
Copy link
Author

then a peerDependency i think might be more appropriate, with the minimum supported version of node

@tuler
Copy link

tuler commented Apr 4, 2024

This also add quite a lot of size to install the package.
2Mb from @types/mute-stream and 2Mb from @types/node.

npx howfat @inquirer/core
@inquirer/core@7.1.1 (28 deps, 2.57mb, 385 files, ©MIT)
├── @inquirer/type@1.2.1 (4.54kb, 7 files, ©MIT)
├─┬ @types/mute-stream@0.0.4 (2 deps, 2.01mb, 105 files, ©MIT)
│ ╰── @types/node@20.12.4 (🔗, 1 dep, 2mb, 100 files, ©MIT)
├─┬ @types/node@20.12.4 (1 dep, 2mb, 100 files, ©MIT)
│ ╰── undici-types@5.26.5 (71.34kb, 35 files, ©MIT)
├── @types/wrap-ansi@3.0.0 (3kb, 4 files, ©MIT)
├─┬ ansi-escapes@4.3.2 (1 dep, 132.26kb, 51 files, ©MIT)
│ ╰── type-fest@0.21.3 (116.26kb, 46 files, ©(MIT OR CC0-1.0))
├─┬ chalk@4.1.2 (5 deps, 95.08kb, 33 files, ©MIT)
│ ├─┬ ansi-styles@4.3.0 (2 deps, 49.67kb, 16 files, ©MIT)
│ │ ╰─┬ color-convert@2.0.1 (1 dep, 33.09kb, 11 files, ©MIT)
│ │   ╰── color-name@1.1.4 (6.54kb, 4 files, ©MIT)
│ ╰─┬ supports-color@7.2.0 (1 dep, 11.19kb, 10 files, ©MIT)
│   ╰── has-flag@4.0.0 (4.32kb, 5 files, ©MIT)
├── cli-spinners@2.9.2 (31.36kb, 6 files, ©MIT)
├── cli-width@4.1.0 (4.66kb, 5 files, ©ISC)
├─┬ figures@3.2.0 (1 dep, 14.42kb, 9 files, ©MIT)
│ ╰── escape-string-regexp@1.0.5 (2.63kb, 4 files, ©MIT)
├── mute-stream@1.0.0 (6.28kb, 4 files, ©ISC)
├── signal-exit@4.1.0 (75.16kb, 29 files, ©ISC)
├─┬ strip-ansi@6.0.1 (1 dep, 9.41kb, 10 files, ©MIT)
│ ╰── ansi-regex@5.0.1 (5.48kb, 5 files, ©MIT)
╰─┬ wrap-ansi@6.2.0 (8 deps, 125.4kb, 48 files, ©MIT)
  ├─┬ ansi-styles@4.3.0 (2 deps, 49.67kb, 16 files, ©MIT)
  │ ╰─┬ color-convert@2.0.1 (1 dep, 33.09kb, 11 files, ©MIT)
  │   ╰── color-name@1.1.4 (6.54kb, 4 files, ©MIT)
  ├─┬ string-width@4.2.3 (4 deps, 66.45kb, 28 files, ©MIT)
  │ ├── emoji-regex@8.0.0 (47.12kb, 8 files, ©MIT)
  │ ├── is-fullwidth-code-point@3.0.0 (4.88kb, 5 files, ©MIT)
  │ ╰── strip-ansi@6.0.1 (🔗, 1 dep, 9.41kb, 10 files, ©MIT)
  ╰── strip-ansi@6.0.1 (🔗, 1 dep, 9.41kb, 10 files, ©MIT)

@SBoudrias
Copy link
Owner

Like stated earlier, I'm happy to review this. But we need a solution that'll also work for typescript users.

dependencies appears to be how we need to handle this for now; see here. And you should also refer to the bug it caused in #1241.

@SBoudrias
Copy link
Owner

Done in @inquirer/prompts@7.0.0 and peer releases for individual prompts.

#1569 is the PR with the change. I reimplemented only the types (made them sub-types) required by Inquirer to be able to remove the direct dependency.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants