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

feat: add TypeScript definitions #54

Closed
wants to merge 9 commits into from
Closed

feat: add TypeScript definitions #54

wants to merge 9 commits into from

Conversation

ffflorian
Copy link
Contributor

This closes #48.

@ffflorian
Copy link
Contributor Author

@malept any news regarding this?

@ffflorian
Copy link
Contributor Author

Since there is no update to this, I will re-open DefinitelyTyped/DefinitelyTyped#54338.

Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

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

Thanks for your patience.

Can you please add tests for the TypeScript definitions (via tsd)?

src/index.d.ts Outdated Show resolved Hide resolved
src/index.d.ts Outdated Show resolved Hide resolved
src/index.d.ts Outdated Show resolved Hide resolved
Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

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

Also I have a naming suggestion for the Options interface.

src/index.d.ts Outdated Show resolved Hide resolved
@malept
Copy link
Member

malept commented Jul 27, 2021

Also, can you avoid squashing all of your commits? It makes it difficult to review new changes. The PR will be squashed at the end.

@ffflorian
Copy link
Contributor Author

ffflorian commented Jul 27, 2021

Also, can you avoid squashing all of your commits? It makes it difficult to review new changes. The PR will be squashed at the end.

Sure. Here are the two changes:

You can see the changes by clicking on "force-pushed" in the message "ffflorian force-pushed the ffflorian:feat/types branch...".

src/index.test-d.ts Outdated Show resolved Hide resolved
src/index.d.ts Show resolved Hide resolved
@malept malept changed the title feat: Add TypeScript definitions feat: add TypeScript definitions Jul 27, 2021
src/index.d.ts Show resolved Hide resolved
src/index.d.ts Outdated Show resolved Hide resolved
src/index.d.ts Outdated Show resolved Hide resolved
@ffflorian
Copy link
Contributor Author

@malept can we continue? 🙂

@malept
Copy link
Member

malept commented Jul 30, 2021

Can you merge the main branch into this PR so CI actually runs?

@ffflorian
Copy link
Contributor Author

Unfortunately tsd needs Node.js >=12.

@malept
Copy link
Member

malept commented Jul 30, 2021

I guess this means that this is a breaking change.

@ffflorian
Copy link
Contributor Author

ffflorian commented Jul 30, 2021

Well, tsd is used only for the test, so in my opinion it's not a breaking change because the tests are not needed for the consumer.

@malept
Copy link
Member

malept commented Jul 30, 2021

Eh, might as well update this package anyway. I'll go do that in a separate PR

@threema-danilo
Copy link
Contributor

Eh, might as well update this package anyway. I'll go do that in a separate PR

Now that Node 12 is required since 4.0.0: Was this PR closed by accident?

It would be nice having in-package types.

@threema-danilo
Copy link
Contributor

Note: I just updated the types at DefinitelyTyped: DefinitelyTyped/DefinitelyTyped#63902

This pull request was closed.
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 a TypeScript definition
3 participants