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

Width option type differs from documentation #132

Open
roydukkey opened this issue Feb 18, 2023 · 4 comments · May be fixed by #145
Open

Width option type differs from documentation #132

roydukkey opened this issue Feb 18, 2023 · 4 comments · May be fixed by #145

Comments

@roydukkey
Copy link

If no width is provided, cliui will try to get the current window's width and use it, and if that doesn't work, width will be set to 80.

Source: https://github.com/yargs/cliui/tree/af3145da0ea31738c4715865a6da0ee388a94c74#cliuiwidth-integer

Screenshot 2023-02-18 at 10 12 18

Screenshot 2023-02-18 at 10 12 41

@shadowspawn
Copy link
Member

shadowspawn commented Feb 18, 2023

In theory, yargs/cliui is not including type definitions in the published package. This is for compatibility withyargs/yargs, which has custom types on DefinitelyTyped: #96

In practice, rollup creates build/index.d.cts and that is not excluded from the publish. That is generated from a "dumb" bootstrap file lib/cjs.ts which was not intended to create exported types. The "smart" types, with optional width, are in lib/index.ts.

The "fix" consistent with historical intent is probably to also exclude .d.cts from the publish!

Edit: the extra suffix for definitions causes problems in yargs-parser too: yargs/yargs-parser#452 (comment)

@roydukkey
Copy link
Author

This is for compatibility withyargs/yargs, which has custom types on DefinitelyTyped: #96

Why does this package even care about yargs/yargs? It's not a dependant. If there are important design requirements for yargs/cliui that yargs/yargs expects, shouldn't they be contained in this repo?

@shadowspawn
Copy link
Member

To be clear, my compatibility comment was about shipping types at all, not about (erroneously) making width required.

Why does this package even care about yargs/yargs? It's not a dependant

This package implements the layout used by yargs, and is made available for separate use. When all the yargs family of packages were moved to TypeScript, there were some challenges due to maintaining compatibility with existing DefinitelyTyped typings for yargs/yargs which meant the new internal typings were not included in the packages.

Shipping typings might not be a problem for cliui, and I think can be revisited: #96 (comment)

@shadowspawn
Copy link
Member

Technical note. I just hit this myself when working on cliui, and think the typings for opts need a minor revisit if we do publish them. The factoring for esm support introduced some typing confusion as to whether the opts parameter is optional or need to pass {}, and additionally whether the width property is optional.

  • esm: no types
  • cjs: default function ui (opts: UIOptions)
  • deno: default function ui (opts: UIOptions)
  • internal interface: function cliui (opts: Partial<UIOptions>, _mixin: Mixin)

Based on the internal implementation, both the opt argument and the width property can still be optional for the client.

Side note: the deno code does not currently infer the window width, which could be improved as it is now available: https://deno.land/[email protected]?s=Deno.consoleSize

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 a pull request may close this issue.

2 participants