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: automate installing dependencies #20

Merged
merged 16 commits into from
Feb 23, 2024

Conversation

neutrino2211
Copy link
Contributor

Addresses #18

@neutrino2211
Copy link
Contributor Author

It works in principle but I am unable to understand the testing setup, so I unfortunately could not write tests

@yusukebe
Copy link
Member

Hi @neutrino2211 !

Super cool! I've tried it on my machine, and the command works well. Feeling good! I have some thoughts for it.

Feature Requests

1. Defaults to the package manager that executed create-hono

I don't know if this can be done, but for example, if you run npm create hono, it would be cool if npm was the default package manager among the choices!

2. Change the directory

I think I'd be happier to be in the directory I created when the command finishes.

Testing

Yeah, I know testing it is difficult, but If we can, we should do it.

@sor4chi Do you have any idea how to test this?

@yutakobayashidev
Copy link

I was reading the create-next-app code a while ago and noticed that I can get the package manager in process.env.npm_config_user_agent.

https://github.com/vercel/next.js/blob/8d28d5954e98060e3a478c6b82acb0fb11c5d6f2/packages/create-next-app/helpers/get-pkg-manager.ts

@yusukebe
Copy link
Member

@yutakobayashidev Nice! Then, we can do it.

@neutrino2211
Copy link
Contributor Author

Great, I'll make those changes

@sor4chi
Copy link
Contributor

sor4chi commented Feb 15, 2024

Hi, @neutrino2211
Thank you for this great PR!

I think it's good to use execa and add E2E test.

This is an example of dependencies detect of netlify cli using execa.

@neutrino2211
Copy link
Contributor Author

@yusukebe It seems like it is impossible to change the directory of the underlying shell

https://stackoverflow.com/questions/19803748/change-working-directory-in-my-current-shell-context-when-running-node-script

Although, I feel like a little message that says "To get started, run: cd project-dir && package-manager dev" after everything is done would be a good compromise

@neutrino2211
Copy link
Contributor Author

@sor4chi Thanks, I'll look into that

@yusukebe
Copy link
Member

@neutrino2211

Although, I feel like a little message that says "To get started, run: cd project-dir && package-manager dev" after everything is done would be a good compromise

Sounds great! Let's go with it.

@neutrino2211
Copy link
Contributor Author

@yusukebe I'm just noticing that the getting started commands for templates are not uniform across the board, so we won't be able to give users a specific command to run. And editing all templates to have some sort of conformity might be overkill for such a simple message.

I want to suggest leaving it as a TODO or opening a new issue for it.

@yusukebe
Copy link
Member

I want to suggest leaving it as a TODO or opening a new issue for it.

Okay! This relates to the future of create-hono. I have some thoughts, which I will present later.

In this case, I think it is enough to just display "cd" to the target directory.

@neutrino2211
Copy link
Contributor Author

neutrino2211 commented Feb 21, 2024

Took a while to understand how to use execa with vitest but it's all looking good now. The tests run for each package manager and the existence of a package manager specific lock file is checked.

Also, it currently runs npm/bun/pnpm/yarn without checking if the test runner has these tools installed. Should something be done when we detect that some of them are missing or should we filter what tests we run based on what the runner has available.

@yusukebe
Copy link
Member

Hi @neutrino2211

Awesome. It works well; the package manager that I used will be set as default!

But, CI was not done and failed. Can you fix this?

@yusukebe
Copy link
Member

@neutrino2211

Oops, it still fails. And, in the last step, please run yarn lint:fix && yarn format:fix to lint and format.

@neutrino2211
Copy link
Contributor Author

@yusukebe

There seems to be no format:fix command, just lint:fix. Is format:fix supposed to be a prettier command? If so I can add it as a script, run it then push.

@yusukebe
Copy link
Member

There seems to be no format:fix command, just lint:fix. Is format:fix supposed to be a prettier command? If so I can add it as a script, run it then push.

Ah, sorry! Yeah, the command is prettier --write.

@neutrino2211
Copy link
Contributor Author

Done, everything seems ok 👌

@yusukebe
Copy link
Member

@neutrino2211

Thank you! But, CI is still failing.

Do you know the reason?
https://github.com/honojs/create-hono/actions/runs/8012129779/job/21887011694?pr=20

@neutrino2211
Copy link
Contributor Author

Oh, I see the problem. The package manager detection fails sometimes and I don't catch the errors.

Sorry for all the back and forth, I'll make changes and push.

@neutrino2211
Copy link
Contributor Author

@yusukebe

I think this is it, crossed fingers 🤞

@yusukebe
Copy link
Member

@neutrino2211

Super cool! Awesome work!

Then, let's land it! Thank you very much!

@yusukebe yusukebe merged commit acf3779 into honojs:main Feb 23, 2024
3 checks passed
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.

4 participants