-
Notifications
You must be signed in to change notification settings - Fork 58
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
Added type definitions #105
base: master
Are you sure you want to change the base?
Added type definitions #105
Conversation
generated .d.ts file
I'd love this to get merged as well :) |
@@ -0,0 +1,760 @@ | |||
// Definitions by James George (https://github.com/jamesgeorge007) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think your git blame is sufficient 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is just a convention to credit the author as found within the DefinitelyTyped project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to have TypeScript support too! But I think we need to finetune this a bit more. @tabrindle do you want to support TypeScript from this repository or do you want to "outsource" it into definitely types? 😄
|
||
export function main(e: any, t: any): any; | ||
|
||
interface Config { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some properties fo the config is missing right? Maybe it's nice to add these too:
- Managers
- Utilities
- Servers
- Virtualization
- SDKs
- IDEs
- Languages
- Databases
- Monorepos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, we can also set all properties to "possible undefined" in the interface itself ofc, to fix this one. e.g.
interface Config {
System?: string[];
...
}
export function main(e: any, t: any): any; | ||
|
||
interface Config { | ||
System: string[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great if we have type hints about the available helpers, that way we don't have to read the docs but simply check the autocompletion whats available 😄
I think it's fairly easy to set this up, but it requires some changes. if we define an interface for the helper consts, like browserBundleIdentifiers
, we can do something like:
interface Config {
Browsers: (keyOf helpers.BrowserBundleIdentifiers)[];
}
showNotFound: boolean; | ||
} | ||
|
||
export function run(e: Config, t: Options): any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both config
and options
should be this, right?
function run(config: Partial<Config>, options: Partial<Options>): any
You don't have to specify all config or option keys, you only define what you need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, might be nice if we define the actual output of the run function instead of any. I think this should be string
with any option. AFAIK it's:
showNotFound
=> no effect on return valueconsole
=> no effect on return value{ json: true }
=> json string{ json: false }
=> formatted string
Binaries: string[]; | ||
Browsers: string[]; | ||
npmPackages: string | string[]; | ||
npmGlobalPackages: string[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just like npmPackages
, I think npmGlobalPackages
also accepts a string
.
emmm. Is there no one to deal with this pr? |
Projects using typescript codebase requires to have type definitions within
.d.ts
files. (Alternatively they can be submitted over to theDefinitelyTyped
repository and installed vianpm i @types/envinfo
)