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 #20

Merged
merged 9 commits into from
Nov 20, 2018
Merged

Conversation

Codex-
Copy link
Contributor

@Codex- Codex- commented Sep 8, 2018

Removed my request from DefinitelyTyped and migrated following your guide.

Hopefully this meets the requirements :)

@sindresorhus sindresorhus changed the title Add TypeScript Definitions Add TypeScript definition Sep 12, 2018
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Let me know if there's anything in the style guide that could be clearer or improved. It's still a work-in-progress :)

@Codex-
Copy link
Contributor Author

Codex- commented Sep 12, 2018

Re: the guide. I suppose the main thing I found a little tricky at first was how to use the type assertions correctly, I ended up going through a few other projects to understand the usage more correctly.

@sindresorhus
Copy link
Owner

Also note:

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.

@sindresorhus
Copy link
Owner

You also left out types for the default export.

@Codex-
Copy link
Contributor Author

Codex- commented Sep 12, 2018

Yup I've come to realize this after the comment around the stream options, will sort out soonish :)

@Codex-
Copy link
Contributor Author

Codex- commented Sep 12, 2018

I've hopefully this time covered everything :)

@Codex-
Copy link
Contributor Author

Codex- commented Sep 17, 2018

Mistakes were made...

- hookStd, stdout, stderr: Added correct typings for options.
- HookPromise: Correct Promise<any> to Promise<void>.
@Codex-
Copy link
Contributor Author

Codex- commented Sep 17, 2018

Should be good now.

@Codex-
Copy link
Contributor Author

Codex- commented Sep 17, 2018

Actually I take it back, few issues to look at still, will comment when ready for another review

- index.js: Changed to initialise hookStd, adding the methods to hookStd, and exporting them all appropriately.
- index.d.ts: Updated to single default export instead of overloaded, while the prior appeared to work, it wasn't as clean.
@Codex-
Copy link
Contributor Author

Codex- commented Oct 4, 2018

Updated with the default export and fixed some typings. I think this is more up to scratch now. Sorry for the delays, uni has been crazy busy.

Can you please clarify a couple of things:

If the parameter description just repeats the parameter name, leave it out.
Does this mean I should remove all docstring params that are simply @param transform or that there shouldn't be a description if the description just repeats the param such as @param transform the transform function.

In the readme it specifies unhooks both stdout and stderr but if you use custom or additional streams it unhooks these too, should it in that case read something more along the lines of unhooks both stdout, stderr or any specified streams

Cheers

@sindresorhus
Copy link
Owner

Does this mean I should remove all docstring params that are simply @param transform

Yes

In the readme it specifies unhooks both stdout and stderr but if you use custom or additional streams it unhooks these too, should it in that case read something more along the lines of unhooks both stdout, stderr or any specified streams

Maybe just:

Unhooks the streams in the streams option.

@Codex-
Copy link
Contributor Author

Codex- commented Nov 15, 2018

Just wondering if there were any pending changes you were wanting that I have possibly missed?

@sindresorhus sindresorhus merged commit d697ab9 into sindresorhus:master Nov 20, 2018
@sindresorhus
Copy link
Owner

Thank you :)

sindresorhus pushed a commit that referenced this pull request Nov 20, 2018
@Codex-
Copy link
Contributor Author

Codex- commented Nov 20, 2018

Great, thanks Sindre!

@Codex- Codex- deleted the typings branch March 30, 2019 05:06
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