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

fix: cli encoding issue #1248

Open
wants to merge 34 commits into
base: main
Choose a base branch
from

Conversation

oliverqx
Copy link
Contributor

description

pr to fix cli encoding issue

related issue: #1247

Copy link

vercel bot commented Jan 30, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
screenpipe ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 4, 2025 2:23am

@oliverqx
Copy link
Contributor Author

oliverqx commented Feb 1, 2025

@louis030195 so it took me some time to replicate consistently. I'm not sure whats causing the issue. The behaviour is wild, so its very hard to understand.

  1. Encoding issue does not always occur but once it starts happening it doesn't go away.
  2. Usually starts happening after running @screenpipe/dev create and ctrl+c in the middle of it ?
  3. Encoding breaks ONLY for the components add command, which I also do not understand..
  4. I thought it was related to prompts, the package, but even by using inquirer it does not encode well.

Only way I've been able to get it working, is by switching from brocli to Commander, so I wanted to know if you'd be okay with me doing that.

Apart from fixing the issue, commander has a wider community support. Their last release was only 2 weeks ago. It has 110M weekly downloads (brocli less than 100K). I've also been using Commander for years now and it has never presented weird encoding issues like this one

@louis030195
Copy link
Collaborator

@louis030195 so it took me some time to replicate consistently. I'm not sure whats causing the issue. The behaviour is wild, so its very hard to understand.

  1. Encoding issue does not always occur but once it starts happening it doesn't go away.
  2. Usually starts happening after running @screenpipe/dev create and ctrl+c in the middle of it ?
  3. Encoding breaks ONLY for the components add command, which I also do not understand..
  4. I thought it was related to prompts, the package, but even by using inquirer it does not encode well.

Only way I've been able to get it working, is by switching from brocli to Commander, so I wanted to know if you'd be okay with me doing that.

Apart from fixing the issue, commander has a wider community support. Their last release was only 2 weeks ago. It has 110M weekly downloads (brocli less than 100K). I've also been using Commander for years now and it has never presented weird encoding issues like this one

would be great to switch to commander

@oliverqx
Copy link
Contributor Author

oliverqx commented Feb 2, 2025

@louis030195 wdyt about me adding some tests? i see theres a folder already but there are no tests yet

edit: would be using jest

@oliverqx oliverqx marked this pull request as ready for review February 3, 2025 17:36
- @drizzle-team/brocli in favour of commander
- kleur in favour of chalk
- prompts in favour of inquirer
@louis030195
Copy link
Collaborator

is it mergeable?

@oliverqx
Copy link
Contributor Author

oliverqx commented Feb 4, 2025

@louis030195 it is yes. changes:

  1. commander migration
  2. implemented logger
  3. refactored create command
  4. create pipe command (for pipes)
  5. create app command (for tauri/electron templates)
  6. using simpleGit now for better nodejs\process support

@oliverqx
Copy link
Contributor Author

oliverqx commented Feb 4, 2025

as always, i've tested it thoroughly. Will keep an eye out for edge cases/bugs but I'm fairly sure its alright. sorry about past bugs

@oliverqx
Copy link
Contributor Author

oliverqx commented Feb 4, 2025

was thinking we could refactor DX into

  1. @screenpipe/dev pipe [create,register,publish]
  2. @screenpipe/dev app [create, x]

x could be some app template specific command to run, and happy to add tests if you guys want them (jest)

lmkwyt!

@louis030195
Copy link
Collaborator

image

@louis030195
Copy link
Collaborator

@screenpipe/dev app create [name]

make sense

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