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

CLI dev mode #436

Merged
merged 18 commits into from
Aug 19, 2023
Merged

CLI dev mode #436

merged 18 commits into from
Aug 19, 2023

Conversation

dgrcode
Copy link
Collaborator

@dgrcode dgrcode commented Jul 20, 2023

Description

Add dev mode for the CLI.

The goal is to improve the DX when contributing changes to anything in the "templates/" folder.

How to use

The way this should be used is by running yarn cli project_name --dev. By using the dev flag, the CLI turns into dev mode.

What it does

When in dev mode, we create symlinks instead of copying files from "templates/". That way, the user would see as if the instance project (the project resulting from running the CLI) had been created. The user can also modify the files within the instance project, and see changes in realtime with the normal tooling (yarn dev, etc). However, since the files are actually symlinked to the source files within "templates/", they are actually modifying the source files.

Once the user is happy with the changes, they can run git diff, and git actually understand that the files changed were the source ones, so there's no extra copy/paste required to update source files.

Caveat: special files

In the normal CLI mode, there are a few files that are not copied, but generated from the source files instead. Those files are package.json and **.templates.mjs.

Even though there are ways to automate those changes as well, it's quite complex to update the right files in the source. Because this is a proof of concept, we won't do any automation, and these special files have to be updated manually.

However, we'll generate sibling files to them with information that aims to make those manual changes easier. Think of those files as a human readable sourcemap for generated files. These files will have the name resulting_file.ext.dev, so they should live next to the resulting files, only with the suffix .dev.

Tasks

  • Add dev flag
  • Create symlinks instead of copying when the dev flag has been sent
  • Add relevant content to package.json.dev files
  • Add relevant content to **.dev files generated out of templates.

@dgrcode dgrcode mentioned this pull request Jul 20, 2023
11 tasks
@dgrcode
Copy link
Collaborator Author

dgrcode commented Jul 20, 2023

force pushed after rebasing on current cli, to get the templates from #430

@dgrcode dgrcode marked this pull request as ready for review July 21, 2023 08:54
@dgrcode
Copy link
Collaborator Author

dgrcode commented Jul 21, 2023

@carletex @technophile-04 I've given this a test locally and things seem to be working as expected.

There are improvements around the .dev file, but we can make them as we need them, which might be never haha.

@carletex
Copy link
Member

carletex commented Jul 21, 2023

This is looking promising!! Thanks for the detailed explanation @dgrcode

Still testing, but noticed a couple of things:

@carletex @technophile-04 I've given this a test locally and things seem to be working as expected.

It seems that the package.json files inside hardhat/foundry are not being copied. Noticed when tried to run yarn chain and it failed Usage Error: Workspace '@se-2/hardhat' not found. Not sure how it can work for you, maybe it's specific to my OS?

This happens with the --dev flag and without it,

@technophile-04
Copy link
Collaborator

It seems that the package.json files inside hardhat/foundry are not being copied. Noticed when tried to run yarn chain and it failed Usage Error: Workspace '@se-2/hardhat' not found

Yeah I faced same issue

Also, I am getting this warning "Unresolved dependency" when I do yarn dev :
Screenshot 2023-07-24 at 9 40 50 PM

But this is looking great @dgrcode !! And great description !! thanks 🙌.... Will try to play around / debug more tomorrow 🙌

@dgrcode
Copy link
Collaborator Author

dgrcode commented Jul 25, 2023

hmm interesting. I'll have a look at this after the issue with windows

@dgrcode
Copy link
Collaborator Author

dgrcode commented Jul 25, 2023

alright so there was a bug where the package.json were not created. I'm not sure what I tested. Probably tested before introducing the bug, and then only tested the symlinks and not the project.

Anyways, the interesting part is that after the package.json are created again, the package "@se-2/hardhat" exists, but npm says it's missing.

It also looks like the package.json from the root folder is symlinked (where it shouldn't), which might be causing npm to look at the packages under the source's "template/base/", instead of the instance's "packages/". It sounds weird that it behaves that way, but I guess there are a few things that can work funny using symlinks.

I'll see if not using symlinks for package.json files fixes it.

@dgrcode dgrcode marked this pull request as draft July 27, 2023 08:46
@dgrcode
Copy link
Collaborator Author

dgrcode commented Jul 27, 2023

I just pushed a couple of commits that made some packages "invisible", but I'm still having funky behavior when trying to run next

@dgrcode dgrcode marked this pull request as ready for review August 3, 2023 10:41
@technophile-04
Copy link
Collaborator

technophile-04 commented Aug 5, 2023

Just tested it out and works like a charm ✨ !!! Tysm @dgrcode !!

I think we missed something duringa6610a5 so it breaks... added a notion doc link in TG chat but if you checkout to d5860a9 everything works there!

Some basic testing which I performed :

  • Tried all the templates with --dev mode and everyone generates nicely, also all the commands like yarn chai etc work!

  • I tried changing YourContract.sol and other frontend files and they were getting reflected in CLI templates files as expected, love it 🔥!

  • Tried adding a new package react-icons in frontend and it works as expected nothing breaks. As discussed in "Caveat: special files" of CLI dev mode #436 (comment) I think it's ok we can update package.json manually since it will be very less changes 🙌

  • Also tested with .template.mjs checkout this branch -> cli-dev-test, here I tried templating index.tsx which updates page Heading with package name selected ... And it works nicely too !!!

Just one question on generated index.tsx.dev file (this was generated for me) :

--- TEMPLATE FILE
templates/base/packages/nextjs/pages/index.tsx.template.mjs


--- ARGS FILES
	- undefined


--- RESULTING ARGS
	- pkgName:	[Hardhat]

Regarding ARGS FILES section I undefined was that expected? Or I should have got the index.tsx.args.mjs filePath there ? (I might be completely wrong here!!)

Nevertheless, everything works great !!! Let's fix the latest merge issue and maybe update CONTRIBUTING.MD a bit, Tysm Dani this is really nice !!!

@dgrcode
Copy link
Collaborator Author

dgrcode commented Aug 7, 2023

Thanks for the tests Shiv! 🙌

I think we missed something duringa6610a5 so it breaks

I'll check what's going on an try to push a fix.

Regarding ARGS FILES section I undefined was that expected?

Nope, you're right. There we should have a path to the file instead of undefined. That should be easy to fix.

I'll see if I can get both those points done today.

Thanks again!

@dgrcode
Copy link
Collaborator Author

dgrcode commented Aug 8, 2023

@technophile-04 I can't reproduce the issues shown in the notion doc.

I was looking at the changes from the cli branch since this branch was created until the last merge, but I didn't see anything suspicious. The diff for reference: https://github.com/scaffold-eth/scaffold-eth-2/compare/686db1a..b5e3c86

Can you re-test and see if you still have the issue?

Just in case I'll mention how I run the command:

  1. I have a terminal in scaffold-eth-2/, with the branch cli-dev-mode checked out
  2. I move to the parent folder cd ..
  3. I run the cli from there node scaffold-eth-2/bin/create-dapp-se2.js test-dev --dev

I'm not sure if we're doing something differently.

@technophile-04
Copy link
Collaborator

The way I usually run CLI is I have two panes split :

  1. one running yarn dev so that it auto compiles and put the changes in dist directory
  2. yarn cli to test cli

Screenshot 2023-08-08 at 9 58 18 PM


Additionally, if you try to do yarn build I get this error :

Screenshot 2023-08-08 at 10 00 00 PM


  1. I move to the parent folder cd ..
  2. I run the cli from there node scaffold-eth-2/bin/create-dapp-se2.js test-dev --dev

Maybe I think you have the prvious build version which you are using here,

could you do yarn build and then do cd .. and node scaffold-eth-2/bin/create-dapp-se2.js test-dev --dev ?

@dgrcode
Copy link
Collaborator Author

dgrcode commented Aug 8, 2023

oh yes, my bad! I forgot to build after pulling the latest changes! 😅🤦

I'll build and see what I find

@dgrcode
Copy link
Collaborator Author

dgrcode commented Aug 8, 2023

yep, reproducing now 😓

@dgrcode
Copy link
Collaborator Author

dgrcode commented Aug 8, 2023

Alright, found the issue with the argsFilesPath. I changed the variable name in the cli branch, and then used that variable (with old name) in the cli-dev-mode branch. The merge didn't find any conflict because that happened in different lines. Sneaky. [reference]
image

I think this has also fixed the "ARGS FILES" section being undefined in the *.dev files.

After fixing it I've tested:

  • Without framework. It works. I initially got an hydration error, but after deleting and restarting, it didn't happen again.
  • With hardhat. Working without any issue.
  • With foundry. Working without any issue.

I can't reproduce the .DS_Store issue. Maybe because I'm on a Mac?

I'll commit the fix for the rename. Could you check again if the .DS_Store went away in the next commit?

Thanks!

@technophile-04
Copy link
Collaborator

technophile-04 commented Aug 8, 2023

Tysm @dgrcode, I just tested and works great even with .DS_Store files present 🙌

Umm I still got "ARGS FILES" section undefined, you can check it in scaffold.config.ts.dev since there we are already using templating logic

Also, from #436 (comment) this is still there :
Screenshot 2023-08-09 at 4 34 48 AM

I see this dosen't cause any harm and everything works correctly but I found doing this solves this issue :

import { promises } from "fs";

const { mkdir, link } = promises;

checkout this stackoverflow link

I think we are pretty close to merging this beast !!


cc @Pabl0cks whenever you have time could you just try this on windows 🙌

@dgrcode
Copy link
Collaborator Author

dgrcode commented Aug 10, 2023

Regarding "ARGS FILES" section, I just created the dev mode projects with hardhat, foundry, and no framework. I'm not getting undefined. My results:

Hardhat
--- TEMPLATE FILE
templates/base/packages/nextjs/scaffold.config.ts.template.mjs


--- ARGS FILES
	- templates/extensions/hardhat/packages/nextjs/scaffold.config.ts.args.mjs


--- RESULTING ARGS
	- chainName:	[hardhat]
Foundry
--- TEMPLATE FILE
templates/base/packages/nextjs/scaffold.config.ts.template.mjs


--- ARGS FILES
	- templates/extensions/foundry/packages/nextjs/scaffold.config.ts.args.mjs


--- RESULTING ARGS
	- chainName:	[foundry]

No framework
--- TEMPLATE FILE
templates/base/packages/nextjs/scaffold.config.ts.template.mjs


--- ARGS FILES
(no args files writing to the template)


--- RESULTING ARGS
(no args sent for the template)

Regarding the unresolved dependency, I completely missed that one. I'll implement the fix you propose, thanks! 👍

@damianmarti
Copy link
Member

I tested it and everything is working fine!!

Anyway, I think that since this dev feature will be used mostly by us at least at the beginning, we can use it carefully, trying to find new bugs and new improvements to make.

I think we should update the CONTRIBUTING.MD file with clear instructions on how to use the cli in dev mode, test the cli on Windows, and if everything goes well, we can merge it!

Great work guys!!!

@Pabl0cks
Copy link
Collaborator

I tried CLI dev mode with Windows and it worked nice.

Tried selecting both Hardhat and Foundry, changing stuff on a few pages and into the contract and all got reflected on templates/ from the source when I did git diff or checked in GitHub Desktop.

Good job! 🙌

@technophile-04
Copy link
Collaborator

technophile-04 commented Aug 14, 2023

I tried CLI dev mode with Windows and it worked nice.

Awesome, tysm 🙌 !!

I think we are just remaining on :

We can update CONTRIBUTING.MD in different PR but since we already published a version after #483, we are not in a rush 🙌

@dgrcode
Copy link
Collaborator Author

dgrcode commented Aug 17, 2023

We moved CONTRIBUTING to "templates/base/", but that's going to be the CONTRIBUTING of the project created by people. Should we have a different CONTRIBUTING at the root? or maybe a copy so it's both in our project and the project created by users?

I'm thinking the same about the root's README.md. Should we rewrite both the README and CONTRIBUTING taking RFC-extensions into account?

@technophile-04
Copy link
Collaborator

We moved CONTRIBUTING to "templates/base/", but that's going to be the CONTRIBUTING of the project created by people. Should we have a different CONTRIBUTING at the root? or maybe a copy so it's both in our project and the project created by users?

We are probably gonna remove "template/base/CONTRIBUTING.md" since people can create their own according to their needs, So I think we should create our CONTRIBUTING.md at the root

I'm thinking the same about the root's README.md. Should we rewrite both the README and CONTRIBUTING taking RFC-extensions into account?

I was thinking something like this :

Root README.md

This will contain a small description of SE-2 and Quickstart similar to what we have in our main README.md and in the end, it will point to CONTRIBUTING.md.

Root CONTIRBUTING.md

  1. Some description like it's under active development, things to keep in mind before PR/issue like some points from main CONTRIBUTING.md
  2. How to run the project locally like requirements
  3. A small desc about --dev and how to use it, a summarised version of CLI dev mode #436 (comment)
  4. Also in the end or in the middle we can point them contributors/developer's guide.

RFC-extension renamed to Conitburotrs/developer_guide.md :

  1. I see RFC-extension as a nice reference book for CLI it has everything in depth and internal working/structure, so I prefer not removing it maybe just updating it a bit.
  2. I think this has all stuff already nicely documented and we can keep updating it as we move into future 🙌, We can also link some of its header Templated Files etc in CONTRIBUTING.md wherever required

Was just thinking out loud and would love to hear other's opinion 🙌


For this PR :

Maybe we just create CONTRIBUTING.md at the root and just have a section "Getting Started" which tells you :

  1. Clone and cd into repo
  2. start a watcher by running yarn dev
  3. In new terminal run yarn cli project-name --dev mode
    After this point we just summarise CLI dev mode #436 (comment) like what it does and how to make changes and and commit.

We can cover other sections of CONTIRBUTING.md other PR, what do you think?

@dgrcode
Copy link
Collaborator Author

dgrcode commented Aug 18, 2023

Yep that sounds great Shiv 🙌

I'll put something together and push a commit

@dgrcode
Copy link
Collaborator Author

dgrcode commented Aug 18, 2023

I added an initial version. To see the changes made to the copied version of CONTRIBUTING.md, I recommend looking at the changes from the two last commits together: https://github.com/scaffold-eth/scaffold-eth-2/compare/bbff792..cli-dev-mode

I'll update the links to the resulting urls after having pushed them to the remote

Copy link
Collaborator Author

@dgrcode dgrcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EDIT: I guess we'll have to update those links as we merge branches

CONTRIBUTING.md Outdated Show resolved Hide resolved
contributors/DEVELOPER-GUIDE.md Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@dgrcode dgrcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like github supports relative links!

CONTRIBUTING.md Outdated Show resolved Hide resolved
contributors/DEVELOPER-GUIDE.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@technophile-04 technophile-04 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tysm @dgrcode for updating .md files, Love it as always !!!!!

I have just removed hardcoded test-cli from package.json leaving it upon on developer add path while testing, because I like to prefer creating "instance project" out of this repo 😅 just to be safe and avoid committing test-cli directory :

Screenshot 2023-08-19 at 7 13 40 AM

We could have added test-cli in .gitignore but I think its fine if we let the developer decide 🙌, also update Developer_Guide reflecting these changes at 8945977. But we can always iterate on it as well as other .md files later

Mergin this, I have gone through the test again and everything works nicely. Thanks again !!! Also thanks to everyone for testing 🙌

@technophile-04 technophile-04 merged commit 4a8c26e into cli Aug 19, 2023
@technophile-04 technophile-04 deleted the cli-dev-mode branch August 19, 2023 02:35
@dgrcode
Copy link
Collaborator Author

dgrcode commented Aug 19, 2023

Yeah makes sense. I didn't think about that use case. Sounds good then! 🙌

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.

5 participants