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.

Sign up for GitHub

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

Jump to bottom

refactor(cli): use citty rewrite cli #1517

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

refactor(cli): use citty rewrite cli #1517

wants to merge 8 commits into from
+441 −240

Conversation

fu050409
Copy link
Member

@fu050409 fu050409 commented Jul 3, 2024

Description:

  • Use citty rewrite cli
  • Refactor all commands to citty
  • Defaults to start a dev server

Resolved: #1123

@fu050409
refactor(cli): use citty rewrite cli
f2ac316 
- Use `citty` rewrite cli
- Refactor all commands to citty
- Defaults to start a dev server

Resolved: farm-fe#1123
@fu050409
chore(changeset): add changeset
8aad3aa
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jul 3, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 688e12f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@farmfe/cli Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@fu050409 fu050409 changed the title Refactor/cli refactor(cli): use citty rewrite cli Jul 3, 2024
@fu050409
Copy link
Member Author

fu050409 commented Jul 3, 2024 •
edited
Loading

Some checks failed, it seems some mistakes happened in my build codes, let me try to fix this

Worked now, waiting for review

@ErKeLost
Copy link
Member

ErKeLost commented Jul 3, 2024

Thanks. I will check the code later.

ErKeLost
ErKeLost reviewed Jul 5, 2024
View reviewed changes
packages/cli/src/index.ts Outdated Show resolved Hide resolved
ErKeLost
ErKeLost reviewed Jul 5, 2024
View reviewed changes
packages/cli/src/index.ts Outdated Show resolved Hide resolved
ErKeLost
ErKeLost reviewed Jul 5, 2024
View reviewed changes
packages/cli/src/index.ts Outdated Show resolved Hide resolved
@fu050409
refactor(cli): divide all commands
fe162a3 
- Remove experimental warning prevention
- Split all commands into separate files
@fu050409
fix(cli): manually handle shared args
688e12f
@fu050409
Copy link
Member Author

fu050409 commented Jul 5, 2024 •
edited
Loading

@ErKeLost Hi! I created three issues in citty ( unjs/citty#152, unjs/citty#153 and unjs/citty#154 )。

Do you think we should manually deal with the problem caused by the global parameter first, or do you think we should wait for upstream to implement these features before we deal with this pr?

@ErKeLost
Copy link
Member

ErKeLost commented Jul 5, 2024

@ErKeLost Hi! I created three issues in citty ( unjs/citty#152, unjs/citty#153 and unjs/citty#154 )。

Do you think we should manually deal with the problem caused by the global parameter first, or do you think we should wait for upstream to implement these features before we deal with this pr?

I think we can wait for a while. We originally wanted to change city. One is speed and another very important point is functional style and elegance of code. Now it seems that it has deviated from the original intention. We don’t need to do a lot of compatibility to complete it. This cli is unreasonable. We can see if citty is willing or supports these functions.

@fu050409
Copy link
Member Author

fu050409 commented Jul 5, 2024

OK, I had submitted a pull request to solve one of the issues and waiting for their review. I'll continue on this pull request once these issues of citty are implemented.

@ErKeLost ErKeLost added this to the Planned milestone Jul 7, 2024
@ErKeLost ErKeLost marked this pull request as draft July 11, 2024 03:00
@fu050409 fu050409 marked this pull request as ready for review July 15, 2024 16:24
@fu050409 fu050409 marked this pull request as draft July 15, 2024 16:24
@fu050409
Copy link
Member Author

@ErKeLost Hi! I found that citty have not maintained for a long time, so I started a new project named archons built by napi. This project is based on clap-rs to build fast, powerfull and elegant CLI tools. I will next try to migrate our CLI to archons instead of citty and keep the farm cli adapted to archons!

@ErKeLost
Copy link
Member

I saw this project which is very interesting. The most important thing about whether we can replace the standard is stability. I can listen to @wre232114 thoughts.

@fu050409
Copy link
Member Author

I saw this project which is very interesting. The most important thing about whether we can replace the standard is stability. I can listen to @wre232114 thoughts.

I have tested on all different platforms with env of node 18 and node 20, it works properly. Of course I'm still not sure that napi will work stably, but if you have the time I'm glad if you can try it out 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ErKeLost ErKeLost ErKeLost left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Planned
Development

Successfully merging this pull request may close these issues.

refactor: @farmfe/cli
2 participants
@fu050409 @ErKeLost