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

Adding typescript declaration #30

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

ivstiv
Copy link
Contributor

@ivstiv ivstiv commented Jul 11, 2023

Changes Made

This project needs more love! It is a great idea and I am surprised there are not many good alternatives. I cobbled together some types to make the library typescript ready. It would be great if you can give some feedback and check over my changes.

Side note: If I nick your code and rewrite the library in typescript from the get-go would you be open for a large contribution or should I just publish my own package? Great name also! 😄

Potential Risks

Potentially a misaligned type can cause confusion in typescript projects.

Test Plan

It is a non-runtime change, so I guess the only way to test it is to pull the repository locally.
Create a dummy typescript project.
Add this to a pnpm package.json file

"dependencies": {
    "redfour": "link:../redfour"
  },

And copy paste the example from the readme in order to see if it works.
If all of that seems too much effort I can spin up a temporary project repository so you can pull and test the changes just let me know.

Checklist

  • [NA] I've increased test coverage
  • Since this is a public repository, I've checked I'm not publishing private data in the code, commit comments, or this PR.

@bradvogel bradvogel self-requested a review July 12, 2023 12:38
@bradvogel
Copy link
Contributor

@ivstiv Thanks for the contribution!

If I nick your code and rewrite the library in typescript from the get-go would you be open for a large contribution or should I just publish my own package

Please! We'd be happy to take this contribution!

If you're willing to go ahead with the TS rewrite now, we can pause on merging this PR. What do you think?

@ivstiv
Copy link
Contributor Author

ivstiv commented Jul 12, 2023

I am sorry but with a full time job I won't be able to start the TS rewrite now, that's why I added the declaration file. I have a few projects that I've been chipping away on the weekends that need finishing, so I won't be able to immediately start. But I will keep it in the back of my head and open a draft PR when I start working, so you can see the progress.

In the mean time I made today this repository to ease the test of my changes for the current PR. I do hope you have a linux like environment (mac or wsl should be fine). The readme should get you up and running in a few minutes.

Copy link
Contributor

@bradvogel bradvogel left a comment

Choose a reason for hiding this comment

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

No problem. I'll merge this then. Thanks for the contribution. We'll be happy to take any future contributions.

@bradvogel bradvogel merged commit b3f21d3 into mixmaxhq:master Jul 12, 2023
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