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

Added type definitions #105

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jamesgeorge007
Copy link

Projects using typescript codebase requires to have type definitions within .d.ts files. (Alternatively they can be submitted over to the DefinitelyTyped repository and installed via npm i @types/envinfo)

@luisherranz
Copy link

I'd love this to get merged as well :)

@@ -0,0 +1,760 @@
// Definitions by James George (https://github.com/jamesgeorge007)
Copy link
Owner

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 😆

Copy link
Author

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.

Copy link

@byCedric byCedric left a 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 {
Copy link

@byCedric byCedric Dec 8, 2019

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

Copy link

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[];
Copy link

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;
Copy link

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.

Copy link

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 value
  • console => no effect on return value
  • { json: true } => json string
  • { json: false } => formatted string

Binaries: string[];
Browsers: string[];
npmPackages: string | string[];
npmGlobalPackages: string[];
Copy link

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.

@lzm0x219
Copy link

emmm. Is there no one to deal with this pr?

@ryhinchey ryhinchey mentioned this pull request Mar 31, 2020
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.

5 participants