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

Add app create command #404

Merged
merged 4 commits into from
Dec 8, 2021
Merged

Add app create command #404

merged 4 commits into from
Dec 8, 2021

Conversation

flyinglimao
Copy link
Contributor

Description

This PR adds a new command app create for users to create an app based on a scaffold.
It's the submission of onflow/flip-fest#31

To use, execute flow app to check out details
圖片

The only verb is create
圖片

It will clone a scaffold GitHub repo with go-git and provide an interactive UI for users to pick stacks.
A User can start from an example or a template:
圖片

圖片

圖片


For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@codecov-commenter
Copy link

Codecov Report

Merging #404 (a7cc2b0) into master (ebc391e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #404   +/-   ##
=======================================
  Coverage   55.33%   55.33%           
=======================================
  Files          36       36           
  Lines        1874     1874           
=======================================
  Hits         1037     1037           
  Misses        707      707           
  Partials      130      130           
Flag Coverage Δ
unittests 55.33% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebc391e...a7cc2b0. Read the comment docs.

- Name: `path`
- Valid Input: Path

A relative path to the app location. Can be a new folder name or existing empty folder.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good if your write something short about each decision (API, cadence template web template) and what do they mean.

Counting objects: 100% (349/349), done.
Compressing objects: 100% (261/261), done.
Total 349 (delta 55), reused 347 (delta 53), pack-reused 0
? Which do you want to start with? Template
Copy link
Contributor

Choose a reason for hiding this comment

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

Is here something missing in the question?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The question I mean is if a user wants to start from an (example) app that can be built and run (kitty-items) with changing very few or no code or a template have only necessary files (framework, scaffold). Since I'm not a native English speaker, could you recommend a clear and succinct question, please?

Copy link

@mwufi mwufi Nov 1, 2021

Choose a reason for hiding this comment

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

Hm.. maybe you can set it up like this? (idk, maybe look at create-react-app for inspiration!)

Project scaffolding
> Start with sample app
  Minimal

internal/app/create.go Outdated Show resolved Hide resolved
internal/app/create.go Outdated Show resolved Hide resolved
internal/app/create.go Outdated Show resolved Hide resolved
Copy link
Contributor

@sideninja sideninja left a comment

Choose a reason for hiding this comment

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

Great contribution, left some comments, nothing major.

docs/project-app.md Outdated Show resolved Hide resolved
Copy link
Contributor

@sideninja sideninja left a comment

Choose a reason for hiding this comment

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

Good job so far there is only one concern I have with this implementation. Seeing how templates have been setup in a separate repo you then use to get the files I feel it's not good for projects that exists elsewhere (like kitty-items) copying that in your template repo will cause those files to get outdated and I feel it would be better implemented if you would just clone those repos on the fly when using the create functionality. For templates that you have to create manually you can still use your custom repo, but then fix that to a specific version so we can control what is used from our CLI repo. Does that make sense?

@flyinglimao
Copy link
Contributor Author

flyinglimao commented Nov 3, 2021

Good job so far there is only one concern I have with this implementation. Seeing how templates have been setup in a separate repo you then use to get the files I feel it's not good for projects that exists elsewhere (like kitty-items) copying that in your template repo will cause those files to get outdated and I feel it would be better implemented if you would just clone those repos on the fly when using the create functionality. For templates that you have to create manually you can still use your custom repo, but then fix that to a specific version so we can control what is used from our CLI repo. Does that make sense?

Not sure if I got it right.
How about this way: add a file example.json that lists a map from project name to GitHub repo, upon a user want to start from a project, ze can select one of them, and the CLI will remove all files and re-clone the project?

For "fix that to a specific version", do you mean tagging a commit as a version and making CLI clone the version which is updated regularly? or the CLI will execute some command like create-react-app to keep it up-to-date instead of removing files?

@sideninja
Copy link
Contributor

@flyinglimao so the problem I see is that if you will include the actual files in your repository (https://github.com/flyinglimao/flow-app-scaffold) that are then used to "create app" and are cloned locally you will have problems with those files getting outdated. Let's take kitty-items for example, that project is constantly getting updated and there is no need for you to actually clone that project in your repo, you could only have the links to the repos and then clone them on the fly from original source not from your repo. The same goes for other templates, they look like they could be cloned from original source.

@flyinglimao
Copy link
Contributor Author

@flyinglimao so the problem I see is that if you will include the actual files in your repository (https://github.com/flyinglimao/flow-app-scaffold) that are then used to "create app" and are cloned locally you will have problems with those files getting outdated. Let's take kitty-items for example, that project is constantly getting updated and there is no need for you to actually clone that project in your repo, you could only have the links to the repos and then clone them on the fly from original source not from your repo. The same goes for other templates, they look like they could be cloned from original source.

Agree with creating from a project. So the problem will be how to obtain such a list, right? Should I put it in flow CLI or an external repo (for people to recommend a new example project)?
But for creating from the custom stack, I didn't find repos can be cloned. I run create-react-app and some commands like that for the scaffold repo. Besides, we also need to install fcl.js first, right?

@sideninja
Copy link
Contributor

Agree with creating from a project. So the problem will be how to obtain such a list, right? Should I put it in flow CLI or an external repo (for people to recommend a new example project)? But for creating from the custom stack, I didn't find repos can be cloned. I run create-react-app and some commands like that for the scaffold repo. Besides, we also need to install fcl.js first, right?

Correct. You can still use the repo for scaffolds and I will create one for you, but before doing that, I think that most of the web scaffolds you use and have nothing to do with Flow are not necessary. I believe if you want to scaffold a react app developers already have the tools to do that. I don't see a good reason to include those, I would limit scaffolding to kitty-items and to maybe a cadence scaffold with core contracts included and folder structure for cadence in place. Developers can then on top of this scaffold use their own scaffolding for the framework they will use.

@flyinglimao
Copy link
Contributor Author

Correct. You can still use the repo for scaffolds and I will create one for you, but before doing that, I think that most of the web scaffolds you use and have nothing to do with Flow are not necessary. I believe if you want to scaffold a react app developers already have the tools to do that. I don't see a good reason to include those, I would limit scaffolding to kitty-items and to maybe a cadence scaffold with core contracts included and folder structure for cadence in place. Developers can then on top of this scaffold use their own scaffolding for the framework they will use.

What's the next thing I should do? Are you going to create a new repo or I should update mine?

@sideninja
Copy link
Contributor

@flyinglimao here is the repo you can use to transfer all the files in https://github.com/onflow/flow-app-scaffold
You should only put contents in this repo that are unique to flow and are not duplicates of some other repos, fetch those sources like explained.

@kerrywei
Copy link
Contributor

Hello @flyinglimao ! Does Gregor's comment make sense to you? Looking forward to seeing more progress on this PR ❤️

@flyinglimao
Copy link
Contributor Author

Hello @flyinglimao ! Does Gregor's comment make sense to you? Looking forward to seeing more progress on this PR ❤️

Yeah but I'm still thinking about how to keep both kitty-items and empty scaffold at the same time. Besides, sorry for that I'm in the mid-term exam so didn't have much time for it.

@flyinglimao flyinglimao force-pushed the master branch 2 times, most recently from 383bf0c to 4016e3e Compare November 14, 2021 08:12
@flyinglimao
Copy link
Contributor Author

flyinglimao commented Nov 14, 2021

https://github.com/onflow/flow-app-scaffold/pull/1
圖片

(Template)
Will clone the scaffold repo and remove some files
圖片

(Example)
Will fetch examples.json from the scaffold repo and clone after selection.
圖片
圖片

@sideninja
Copy link
Contributor

@flyinglimao I merged PR to the app-scaffold repo. Btw I suggest you add all those files to second level so you can have multiple custom scaffolds there, so instead of all files being top level have directories for each custom scaffold like "basic" etc.

@flyinglimao flyinglimao force-pushed the master branch 3 times, most recently from 828ccc0 to daaad8d Compare November 18, 2021 05:34
@flyinglimao
Copy link
Contributor Author

@flyinglimao I merged PR to the app-scaffold repo. Btw I suggest you add all those files to second level so you can have multiple custom scaffolds there, so instead of all files being top level have directories for each custom scaffold like "basic" etc.

圖片
圖片

@sideninja
Copy link
Contributor

@flyinglimao I merged PR to the app-scaffold repo. Btw I suggest you add all those files to second level so you can have multiple custom scaffolds there, so instead of all files being top level have directories for each custom scaffold like "basic" etc.

圖片 圖片

Hi, did you manage to complete any work on this?

@flyinglimao
Copy link
Contributor Author

flyinglimao commented Nov 26, 2021

@flyinglimao I merged PR to the app-scaffold repo. Btw I suggest you add all those files to second level so you can have multiple custom scaffolds there, so instead of all files being top level have directories for each custom scaffold like "basic" etc.

圖片 圖片

Hi, did you manage to complete any work on this?

I think I completed it. Did I miss anything?
In the screenshots, I finish the feature that makes users can choose a scaffold (for now there is only one choice basic) as you said
After merged scaffold PR and the cli should work properly I think

@sideninja
Copy link
Contributor

Ok, I see, I meant to say to also update app-scaffold to include all the code inside basic folder so you can create more custom scaffolds if needed and you can then have directory structure like /basic/{basic files}, /advanced/{...} but this can also change when we add another scaffold, so maybe that is good enough for now.

@flyinglimao
Copy link
Contributor Author

It's what the PR in scaffold repo for, the newer structure can be previewed in my repo. Or did I misunderstand something?

@sideninja
Copy link
Contributor

sideninja commented Nov 26, 2021

If I run this command flow app create . and choose Template it then asks me to choose api/cadence/web. You should create all of those folders including flow.json on the top-level when choosing template. I would also advise changing Template to Empty Flow Project and Example to Kitty Items Example. After that we can release it.

@flyinglimao
Copy link
Contributor Author

flyinglimao commented Nov 26, 2021

If I run this command flow app create . and choose Template it then asks me to choose api/cadence/web. You should create all of those folders including flow.json on the top-level when choosing template. I would also advise changing Template to Empty Flow Project and Example to Kitty Items Example. After that we can release it.

That's because the PR isn't merged. The CLI will consider all folders in the repo as a template and now there are api, cadence, web but will be basic after the PR is merged

@sideninja
Copy link
Contributor

My apologies, I completely overlooked that PR, it's merged now. Will test more and release in the next version. Thank you.

@sideninja sideninja merged commit cad7390 into onflow:master Dec 8, 2021
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.

6 participants