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/ts migration #216

Merged
merged 12 commits into from
Apr 20, 2024
Merged

Feat/ts migration #216

merged 12 commits into from
Apr 20, 2024

Conversation

ilteoood
Copy link
Collaborator

Closes #212
Closes #202

With this PR I aim to migrate everything to ts, including build steps and lint configurations.

@ilteoood ilteoood requested a review from simoneb April 18, 2024 06:43
@simoneb simoneb requested a review from karl-power April 18, 2024 07:47
@ilteoood ilteoood requested review from karl-power and simoneb April 18, 2024 17:20
Copy link

@karl-power karl-power left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@simoneb simoneb left a comment

Choose a reason for hiding this comment

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

@ilteoood have you tested that this works with your changes? I wouldn't want to merge and release a new version just to realize that it doesn't work. I know that testing a CLI package is not the simplest thing, but just checking if you tried

@ilteoood
Copy link
Collaborator Author

ilteoood commented Apr 19, 2024

@simoneb I tried the scripts included in the package.json, which involves some examples provided, but nothing more

@simoneb
Copy link
Member

simoneb commented Apr 19, 2024

@ilteoood can you try using it as a binary, in the same way as it would be used by the end user?

@ilteoood
Copy link
Collaborator Author

@ilteoood can you try using it as a binary, in the same way as it would be used by the end user?

Hi @simoneb, I just tried it by both:

  • linking the package with npm link;
  • publishing the package on a local instance of verdaccio and running it through npx.

In both the scenario it is working fine. IMO we are ready to merge

@ilteoood ilteoood requested a review from simoneb April 20, 2024 07:22
@simoneb simoneb merged commit 66c6809 into master Apr 20, 2024
3 checks passed
@simoneb simoneb deleted the feat/ts-migration branch April 20, 2024 08:56
@github-actions github-actions bot mentioned this pull request Apr 23, 2024
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.

Consider switching to TS
3 participants