search

sindresorhus / alfy

Create Alfred workflows with ease
MIT License
2.63k stars 104 forks source link

Add TypeScript definitions #138

Closed jopemachine closed 3 years ago

jopemachine commented 3 years ago

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

sindresorhus commented 3 years ago

Make sure you follow https://github.com/sindresorhus/typescript-definition-style-guide

jopemachine commented 3 years ago

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?

LitoMore commented 3 years ago

@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 commented 3 years ago

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

jopemachine commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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?

LitoMore commented 3 years ago

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 commented 3 years ago

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.