-
Notifications
You must be signed in to change notification settings - Fork 936
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
CLI initial version #413
CLI initial version #413
Conversation
from https://github.com/carletex/create-dapp-example Co-authored-by: Shiv Bhonde <[email protected]> Co-authored-by: Daniel González Reina <[email protected]>
Went for
Could you guys try? @sverps @rin-st @Pabl0cks @damianmarti (not tagging @technophile-04 because he is next to me IRL rn :D) |
weird spacing with the other one :(
@carletex this is awesome!! :-) I tested it trying to think that I'm a new user of SE-2 and I think that these improvements can be good things to do:
I used the installed se-2 project and everything seems fine. We are close to having the cli ready!! 🥳 |
Hey, great job! For now I'm getting an error when trying to preinstall packages for both hh and foundry (Install packages? y). Part of the log:
and
so something wrong with Other than that noticed that foundry project readme should be changed, but it's already mentioned. And name of the local network for foundry shouldn't be |
@rin-st did you get this when doing it using Because we were getting this error when you are trying it out locally using eg :
We need to document this problem in README while working locall....But could you please confirm if you got this error while doing |
@technophile-04 I have an error in both |
btw I was trying to use |
task: () => | ||
copyTemplateFiles(options, templateDirectory, targetDirectory), | ||
}, | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changing order to initializing git
-> installing deps
fixes my errors for all cases
Awesome job! Was exciting to see @carletex and @technophile-04 working on this IRL 🚀 On Windows (both my desk and laptop) it's working like a charm when I choose hardhat, but when I choose foundry, I got problems to
|
Thanks for the feedback and the help @rin-st.
Yep, this was happening when running it inside an existing git repo (because of the postinstall yarn hook). Your fix works perfectly, but it leaves the ** This is expected since we have an empty
Thanks @Pabl0cks ! Yes, this happens because people need to install foundryup (that contains anvil and some other tools) in their system. What we are going to do is check if they have it installed and if not show a message hinting them to install. We'll be pushing (& publishing) in a bit. Thanks all! |
Thx @carletex I had no idea about that. Now with foundryup is working great! I'll send a nitpick edge case:
That's all I get with foundry:
|
Yes @Pabl0cks! I think Shiv solved it here: 9bb9422 I haven't published it yet to NPM but it should work if run the CLI locally (instead of create-eth) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having a look at the PR. Some thoughts/questions:
- Shouldn't we commit the
dist/
folder? I'm curious how npx is working without the dist folder. Do they run the build task on their servers? 🤷 - What do you think about committing the
.vscode/
folder to keep the debugger configuration? It definitely helps me, and I guess it could help anybody trying to debug the cli. - I've talked with @carletex about the two files that need to be renamed:
.env-default
and.gitignore-keep
. I'll convert them to templates so we can skip the renames and avoid exception for certain files.
TODO list for myself:
- Fixed by 2599a95
Use templates for renamed files - Should be fixed by Remove unused types #432. Remove unused types from
src/types.ts
- Fixed by 2f36b2e.
Look into whyfindFilesRecursiveSync
wasn't working for.gitignore-keep
and.env-default
. Even though we won't need to rename files, it might have a bug that's not showing up and could break something in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can update the smartContractFramework legacy stuff from here so it works with the current setup. I'll look into it.
message += ` | ||
\t${chalk.bold("Start the local development node")} | ||
\t${chalk.dim("yarn")} chain | ||
`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just something minor to keep consistent indentation. The indented 4 spaces here are added before the \t
, but down below at line 29 for example, we have 6 spaces before the \t
. We might want to use \t\t
without spaces on the left to always keep the same spacing.
Yep, it builds there
We decided against it in SE-2. Not everyone uses VSCode (eg. I use Intellij and Shiv uses Vim). The same way that I don't want to commit the The ToDos look great and pretty straightforward. Also remember to take a look at the "how to contribute" stuff (hot reloading, symlinks, different repo) which is the most important piece. |
Cli weekly backmerge
Co-authored-by: Shiv Bhonde <[email protected]>
Closing this in favor of https://github.com/scaffold-eth/create-eth As we discussed, the CLI version will live in its own repo (GitHub actions already publishing to NPM in that repo) |
NOTE: Further changes to this branch should be done by a PR
This PR changes the workflow for users to use scaffold-eth-2. Instead of cloning, users will use this CLI (npx create-whatever-name@latest). It'll let users pick different options (hardhat/foundry + extensions)
Initially, we'll start with hardhat/foundry (so we can merge and iterate), but @dgrcode built an extension system. Read more here
Some ToDos (maybe we should create some issues?):
Better ASCII art (or just chalk with color and fancy chars) on the intro message. (render-intro-message.ts)Fixed 42e26bbIn the outro message hint about yarn chain / yarn deploy / yarn start (render-outro-message.ts). Fixed da43950Fix foundry template: Let's remove the need of having to create a .env file manually (the cli can do it for you... for now, until we find a system for extensions to add their own .env)Fixed 8e05bbbWe don't need to solve all of them before merging.
PS: There are a lot of "files changed", but most of them are just moving things to the template folders (which git is detecting :))