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(typescript): convert js to ts #31

Merged
merged 1 commit into from
Nov 7, 2022
Merged

Conversation

tduyng
Copy link
Contributor

@tduyng tduyng commented Nov 29, 2021

Close #28

  • Convert js to ts
  • Improve some types
  • Update examples files
  • Create make command to run examples files

@tduyng tduyng force-pushed the OAC-0004-ts branch 5 times, most recently from a8bfe62 to 3aa41a8 Compare December 1, 2021 11:03
@tduyng tduyng force-pushed the OAC-0004-ts branch 3 times, most recently from 12dfee3 to e404853 Compare July 27, 2022 10:30
@tduyng tduyng force-pushed the OAC-0004-ts branch 7 times, most recently from 6b22f9a to 50cec13 Compare July 27, 2022 12:00
@pebie
Copy link
Contributor

pebie commented Jul 27, 2022

@tienduy-nguyen,
Now that we have to build the project, how did you plan to publish the build on npm. It seems that you have test it on your own npm registry. Could you explains how you did that ?

@tduyng
Copy link
Contributor Author

tduyng commented Jul 27, 2022

@tienduy-nguyen, Now that we have to build the project, how did you plan to publish the build on npm. It seems that you have test it on your own npm registry. Could you explains how you did that ?

Yes, I tested it in my own npm.

@tduyng tduyng force-pushed the OAC-0004-ts branch 2 times, most recently from 35f92e2 to e7be794 Compare July 27, 2022 13:28
@tduyng tduyng changed the title feat(typescript): convert core files to ts feat(typescript): convert js to ts Jul 27, 2022
.gitignore Outdated Show resolved Hide resolved
.npmignore Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/global.d.ts Outdated Show resolved Hide resolved
src/importer.js Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/output_utils.ts Outdated Show resolved Hide resolved
src/wrapper.mjs Outdated Show resolved Hide resolved
@pebie pebie mentioned this pull request Jul 27, 2022
@tduyng tduyng force-pushed the OAC-0004-ts branch 2 times, most recently from 65db764 to 2a50d03 Compare July 27, 2022 16:43
@fthouraud
Copy link
Contributor

Thank you for your contribution @tienduy-nguyen.

If I may, this work should have been split into several PR to ease the review and track each issue.

Also, why do you replace ava with Jest? 🤔

@pebie
Copy link
Contributor

pebie commented Jul 28, 2022

If I may, this work should have been split into several PR to ease the review and track each issue.

What do you mean ? Split between test, core and build maybe ?

@pebie pebie self-requested a review July 28, 2022 14:00
@tduyng
Copy link
Contributor Author

tduyng commented Jul 28, 2022

If I may, this work should have been split into several PR to ease the review and track each issue.
What do you mean ? Split between test, core and build maybe ?

I thinks Fabien want to talk about the separation for steps: travisCI -> Github action, update dependencies, .....

  • I tried did it before with 3 different PR, but after all, il will take so much more efforts and time to do the review. So I deleted other PR and put them all here

Also, why do you replace ava with Jest? 🤔

Ava is very light, It's not too bad for an libary npm. But it's syntax is not too familiar. It demands more tools to work completely as sinon and nyc. And debug with ava is less simple.
Meanwhile, Jest is a really good tool. The features are complete. It's easy to use, syntaxe is really well known and familiar. And Jest has many improvements for the performance in recents version.

On another hand, we have a trouble with ava with Github action when we make the comparison the string with color.

So I think Jest is a good choice to replace ava.

@fthouraud
Copy link
Contributor

It should have been done sequentially to keep track of each major modification instead of only two commits covering a total rewrite of the lib, some addition, a new CI system, etc.

The advantage of AVA is especially that it is a lightweight test runner without bringing extra tooling. Also, the fact you're not familiar with the syntax a tool provides doesn't mean it has to be replaced. Regarding Jest's improvements, I'm pretty sure it is unnoticeable for such a small project. This change is purely motivated by your personal tastes instead of actual benefits for the library.

I can't speak for the problem you encountered but as long as AVA was working before I bet you looked in the wrong direction ;)

No hard feelings here, I'm glad you contributed but it should have been focused more on the library and its history.

@fthouraud
Copy link
Contributor

It also relates to this #26.

@tduyng
Copy link
Contributor Author

tduyng commented Jul 31, 2022

It should have been done sequentially to keep track of each major modification instead of only two commits covering a total rewrite of the lib, some addition, a new CI system, etc.

The advantage of AVA is especially that it is a lightweight test runner without bringing extra tooling. Also, the fact you're not familiar with the syntax a tool provides doesn't mean it has to be replaced. Regarding Jest's improvements, I'm pretty sure it is unnoticeable for such a small project. This change is purely motivated by your personal tastes instead of actual benefits for the library.

I can't speak for the problem you encountered but as long as AVA was working before I bet you looked in the wrong direction ;)

No hard feelings here, I'm glad you contributed but it should have been focused more on the library and its history.

OK, thank Fabien for clarifying them. I appreciate that. I'll take the modification.

@pebie
Copy link
Contributor

pebie commented Aug 1, 2022

It should have been done sequentially to keep track of each major modification instead of only two commits covering a total rewrite of the lib, some addition, a new CI system, etc.

Ok to split by commit but do multiple PRs seems to be complicated since migrate TS needs to rework eslint, node modules, ci/cd... And I'm not sure this is possible to split it in multiple PR.

I understand this PR is hard to review so we can split it by commit :

  • tests
  • core
  • tooling

Maybe

@fthouraud
Copy link
Contributor

Why should this be difficult?

This should be perfectly fine in this order:

The dependency upgrade could be included in one of the two MR, but if the code needs modifications, this should not be mixed up with other changes.

@tduyng tduyng force-pushed the OAC-0004-ts branch 2 times, most recently from 2a50d03 to 5384069 Compare August 4, 2022 10:15
@tduyng
Copy link
Contributor Author

tduyng commented Aug 4, 2022

Why should this be difficult?

This should be perfectly fine in this order:

The dependency upgrade could be included in one of the two MR, but if the code needs modifications, this should not be mixed up with other changes.

Thank Fabien for your suggestion.
So now I separated in 3 different PR:

Copy link
Contributor

@fthouraud fthouraud left a comment

Choose a reason for hiding this comment

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

Thank you @tienduy-nguyen for your effort 🙏 This is much appreciated.

This should be included in a major release due to the number of changes and the impact on imports.

@fthouraud fthouraud merged commit 732a908 into ekino:master Nov 7, 2022
@fthouraud fthouraud mentioned this pull request Nov 7, 2022
3 tasks
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.

Migrato to TypeScript
3 participants