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 TypeScript definition #13

Merged
merged 10 commits into from Feb 1, 2019
Merged

Conversation

Fazendaaa
Copy link
Contributor

Hey, @sindresorhus.

Due to Hacktoberfest 2018, I'm contributing to some packages.

TypeScript typings help a lot of us who use it due to it's integration to TSLint and some more strict checkings, needed to be performed in some cases in the used packages as well; that's why I've decided to share the ones that I've made, making it more accessible to all programmers in this environment :)

Besides adding this, I would love to add a Husky integration to perform linter checkings, tests and documentation to git hooks. While doing this, if is it of your interest I can rewrite the JS parts in TS also, adding tests and everything.

@sindresorhus
Copy link
Owner

Hey. Awesome to see participation for Hacktoberfest. ✨

Can you follow this styleguide?

@sindresorhus
Copy link
Owner

Besides adding this, I would love to add a Husky integration to perform linter checkings, tests and documentation to git hooks. While doing this, if is it of your interest I can rewrite the JS parts in TS also, adding tests and everything.

I appreciate the offer, but I'm not interested in that.

@Fazendaaa
Copy link
Contributor Author

Hey. Awesome to see participation for Hacktoberfest. sparkles

Can you follow this styleguide?

No problems :)

@Fazendaaa
Copy link
Contributor Author

@sindresorhus what awesome guideline for contributions, I did not know these standards of yours and the first time I see something like that. I learned a lot.

@sindresorhus
Copy link
Owner

For packages with a default export, use export default foo;, not export = foo;. You will have to add module.exports.default = foo; to the index.js file.

The pull request should have the title Add TypeScript definition. (Copy-paste it so you don't get it wrong)

index.d.ts Outdated
export interface ShellEnv {
readonly [x: string]: string;
readonly SHELL: string;
readonly TERM_PROGRAM: string;
Copy link
Owner

Choose a reason for hiding this comment

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

These are no guaranteed to exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks

index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
package.json Outdated
"xo": {
"ignores": [
"*.ts"
]
Copy link
Owner

Choose a reason for hiding this comment

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

This is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay

index.test-d.ts Outdated
import {expectType} from 'tsd-check';
import shellEnv, { sync, ShellEnv } from '.';

(async () => {
Copy link
Owner

Choose a reason for hiding this comment

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

index.test-d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated
* @param shell To read the environment variables from.
* @returns The environment variables.
*/
export function shellEnv(shell?: string): Promise<ShellEnv>;
Copy link
Owner

Choose a reason for hiding this comment

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

This should be a default export only, not a named export too.

index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated
/**
* Get the environment variables defined in your dotfiles.
*
* @param shell To read the environment variables from.
Copy link
Owner

Choose a reason for hiding this comment

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

Document the default value.

sindresorhus and others added 5 commits October 24, 2018 07:48
Co-Authored-By: Fazendaaa <lucas.carotta@outlook.com>
Co-Authored-By: Fazendaaa <lucas.carotta@outlook.com>
Co-Authored-By: Fazendaaa <lucas.carotta@outlook.com>
Co-Authored-By: Fazendaaa <lucas.carotta@outlook.com>
@sindresorhus sindresorhus changed the title Adding TS typings. Add TypeScript definition Feb 1, 2019
@sindresorhus sindresorhus merged commit bec9ece into sindresorhus:master Feb 1, 2019
@sindresorhus
Copy link
Owner

Nice work! :)

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.

None yet

2 participants