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 definitions #138

Merged
merged 22 commits into from
Aug 5, 2021
Merged

Conversation

jopemachine
Copy link
Contributor

@jopemachine jopemachine commented Jul 22, 2021

Add typescript definitions

Thanks for the quick reply!

Here is the PR.

Maybe do a pull request to add types to that package? Link to it here and I'll ping the maintainer to review it.

Ok, I will try to do a pull request to add types to cache-conf.

Related

Fixes #127

@jopemachine jopemachine changed the title Add types Add typescript definitions Jul 22, 2021
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 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

@jopemachine
Copy link
Contributor Author

I'm sorry for missing several conventions guides and thank you for pointing it out.

May I request code review again to find out if there are points that I'm still missing?

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 Show resolved Hide resolved
index.d.ts 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
@LitoMore
Copy link
Sponsor Collaborator

LitoMore commented Jul 29, 2021

@jopemachine Would you mind upgrading xo to 0.39.1 in the pull request? The linter will help you fix all linter-related issues automatically.

@LitoMore
Copy link
Sponsor Collaborator

@jopemachine You could go through the Testing to learn how to use tsd to write tests for your TypeScript type definitions.

@jopemachine
Copy link
Contributor Author

jopemachine commented Jul 29, 2021

@LitoMore, @sindresorhus I appreciate for detailed code reviews :)

I upgraded xo to 0.39.1 and fix some linting errors (New errors after upgrading xo)

And updated type declaration, added tsd test.

In tsd test, to test functions with return type void, I added /* eslint-disable @typescript-eslint/no-confusing-void-expression */ comment on top.

If we don't need to test void returning functions, just let me know, I will remove the comment and tests.

@jopemachine
Copy link
Contributor Author

It seems that update-notifier in xo 0.39.1 uses node >= 10,

So test fails in Node 8.

May I ask how I should handle this?

@LitoMore
Copy link
Sponsor Collaborator

LitoMore commented Jul 29, 2021

Test fails in Node 8.

@jopemachine Feel free to drop Node.js 8 from GitHub Actions.

It seems the got@9 blocked us using Node.js 10 or higher. We could solve this problem later (in another PR) if you are interested.

@jopemachine
Copy link
Contributor Author

jopemachine commented Jul 30, 2021

Test fails in Node 8.

@jopemachine Feel free to drop Node.js 8 from GitHub Actions.

It seems the got@9 blocked us using Node.js 10 or higher. We could solve this problem later (in another PR) if you are interested.

Of course, I'm happy to have the opportunity to contribute to OSS.

It seems that got@12.x is ESM, should we upgrade got to got@11.8.2?

package.json Outdated Show resolved Hide resolved
@LitoMore
Copy link
Sponsor Collaborator

LitoMore commented Jul 30, 2021

It seems that got@12 is ESM, should we upgrade got to got@11.8.2?

@jopemachine Yes. Let's discuss this in the future pull request.

@jopemachine jopemachine mentioned this pull request Jul 30, 2021
@jopemachine
Copy link
Contributor Author

It seems that got@12 is ESM, should we upgrade got to got@11.8.2?

@jopemachine Yes. Let's discuss this in the future pull request.

@LitoMore I made a PR about this issue here.

@LitoMore LitoMore changed the title Add typescript definitions Add TypeScript definitions Aug 2, 2021
index.test-d.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@sindresorhus sindresorhus merged commit 5b909f8 into sindresorhus:main Aug 5, 2021
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.

Add TypeScript definition
3 participants