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

Add PackageJson and LiteralUnion types #5

Merged
merged 13 commits into from
Mar 15, 2019
Merged

Add PackageJson and LiteralUnion types #5

merged 13 commits into from
Mar 15, 2019

Conversation

BendingBender
Copy link
Contributor

Will do the eslint types in a later PR.

index.d.ts Show resolved Hide resolved
npm.d.ts Outdated Show resolved Hide resolved
npm.d.ts Outdated Show resolved Hide resolved
npm.d.ts Outdated
*/
postrestart?: string;

[scriptName: string]: string | undefined;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why allow undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In strict mode, the type of the index signature has to match with the other keys defined in the type. This is the reason I've used the

type Foo = {
  key?: string;
} & {
  [key: string]: string;
}

below.

npm.d.ts Outdated Show resolved Hide resolved
npm.d.ts Outdated
types?: string;

/**
Location of the bundled TypeScript declaration file. Alias of `types`.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use @alias? http://usejsdoc.org/tags-alias.html

npm.d.ts Outdated Show resolved Hide resolved
npm.d.ts Outdated Show resolved Hide resolved
npm.d.ts Outdated Show resolved Hide resolved
npm.d.ts Outdated
*/
jspm?: PackageJson;
} & {
[k: string]: unknown;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[k: string]: unknown;
[key: string]: unknown;

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this in a separate intersection type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sindresorhus
Copy link
Owner

Can you move npm.ts to a source directory? (Don't forget to add source to files in package.json).

source/npm.d.ts Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

I think the filenames for source/test should be npm => package-json

@sindresorhus sindresorhus changed the title Add types for package.json and literal union workaround Add PackageJson and LiteralUnion types Mar 14, 2019
@sindresorhus
Copy link
Owner

Can you add the types to the readme?

@BendingBender
Copy link
Contributor Author

Which ones? Every single one or only the main PackageJson?

@sindresorhus
Copy link
Owner

Only PackageJson and LiteralUnion

@sindresorhus sindresorhus merged commit 0f273d7 into sindresorhus:master Mar 15, 2019
@sindresorhus
Copy link
Owner

Thanks :)

@BendingBender BendingBender deleted the package-json-types branch March 15, 2019 08:51
@sindresorhus
Copy link
Owner

@BendingBender Was wondering if you would like to join this project as a maintainer? No commitment, just want to show your name in the readme since you have contributed a lot of code here and you might have opinions about future type additions.

@BendingBender
Copy link
Contributor Author

@sindresorhus It would be a pleasure for me!

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