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(create-waku): fix cli and swap to clack + pico #1290

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

Conversation

tylersayshi
Copy link
Contributor

prompts => @clack/prompts
kolorist => picocolors

these libraries are copying the pattern set by create-vite

fixes #1289

prompts => @clack/prompts
kolorist => picocolors

these libraries are copying the pattern set by create-vite

fixes dai-shi#1289
Copy link

vercel bot commented Mar 5, 2025

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

1 Skipped Deployment
Name Status Preview Updated (UTC)
waku ⬜️ Ignored (Inspect) Visit Preview Mar 8, 2025 4:44am

Copy link

codesandbox-ci bot commented Mar 5, 2025

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@tylersayshi tylersayshi marked this pull request as draft March 5, 2025 22:51
@tylersayshi
Copy link
Contributor Author

draft to fix e2e test setup

@tylersayshi
Copy link
Contributor Author

tylersayshi commented Mar 6, 2025

@dai-shi would you be ok with testing the cli with execa & vitest like is done with create-vite

https://github.com/vitejs/vite/blob/main/packages/create-vite/__tests__/cli.spec.ts

I am having issues handling the stdin and stdout with clack and vite's test seems like a clean way to solve for this part.

Note: It does not test the interactive mode, but maybe we're ok with just an args based test too now that rob added the args support?

@dai-shi
Copy link
Owner

dai-shi commented Mar 6, 2025

would you be ok with testing the cli with execa & vitest

I'm not very familiar but sounds ok. Any drawbacks?

It does not test the interactive mode, but maybe we're ok

I think we should eventually test the interactive mode, but for now better than nothing.

@tylersayshi
Copy link
Contributor Author

I'm not very familiar but sounds ok. Any drawbacks?

the drawback with unit testing that I thought initially was that it would just not test interactive mode.

to test interactive mode though, we should mimic the @clack/prompts tests. They mock writable and readable to run the tests through vitest.

https://github.com/bombshell-dev/clack/blob/main/packages/core/test/prompts/prompt.test.ts

So I think two tests would be good

  1. test create-waku with args
  2. test create-waku in interactive mode by mocking writable and readable.

Do you think both are needed for this PR or would a follow up be on for one of them?

@dai-shi
Copy link
Owner

dai-shi commented Mar 7, 2025

Do you think both are needed for this PR or would a follow up be on for one of them?

I'll leave it to you. Our requirement is not break the current behavior. (btw, I'm planning another patch release, so let me know if you go with separate PRs.)

@tylersayshi
Copy link
Contributor Author

I'll plan for just this one PR and both tests mentioned above added. I want to be as careful as I can to not cause regression with this.

It will need to be tomorrow though. I'm out for trivia tonight. ✌️

@dai-shi dai-shi mentioned this pull request Mar 7, 2025
@tylersayshi tylersayshi marked this pull request as ready for review March 8, 2025 04:26
@tylersayshi tylersayshi marked this pull request as draft March 8, 2025 04:36
@tylersayshi
Copy link
Contributor Author

planning to wait for when we drop node 18 to mark this ready

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.

create-waku confuses the target dir
2 participants