-
Notifications
You must be signed in to change notification settings - Fork 100
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: basic create-frames cli tool #143
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@davidfurlong ready for review |
how do we test this locally? |
@davidfurlong I did not find a way to do that. What I do is just running the |
1d03a69
to
71e962d
Compare
@davidfurlong I improved the installation of dependencies by providing a comment. Also I improved readme files a little and changed the |
templates/next/app/page.tsx
Outdated
const previousFrame = getPreviousFrame<State>(searchParams); | ||
|
||
const frameMessage = await getFrameMessage(previousFrame.postBody, { | ||
// remove if you aren't using @frames.js/debugger or you just don't want to use the debugger hub |
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.
how does this work with deploying to production?
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.
Hm that would need a check if you are running on NODE_ENV=production
and remove it in that case, if that's what is on your mind.
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.
Added the condition
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.
@davidfurlong is this ok with you?
params.t || | ||
(await select({ | ||
message: "Choose a template for the project", | ||
options: getTemplates().map((template) => ({ |
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.
would be great to have the existing templates here
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.
that way you would need to update the list manually each time.
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 think it is better to keep this as is otherwise it could easily happen that you forget to add a template here.
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.
Ah ok now I think I understand what you mean. Moving templates from starter to standalone templates.
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 added 2 more templates based on starters from examples
folder. Would you like to get rid of examples
altogether? I think that would be better to do in different PR.
we should probably also update the docs to use this |
628c6e5
to
cf263b1
Compare
bb82367
to
d1d5685
Compare
docs/pages/index.mdx
Outdated
<HomePage.InstallPackage name="frames.js" type="install" /> | ||
<HomePage.InstallPackage name="frames" type="init" /> |
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 think we should leave the install as the initial call to action since we have create-frames in the quickstart section
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.
Done
d1d5685
to
7e7b20e
Compare
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.
would be great if we could deduplicate the examples and the templates but that might be out of scope for this PR. otherwise looks good to me!
@@ -0,0 +1,29 @@ | |||
{ | |||
"name": "template-next", |
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.
is there a way for us to maybe deduplicate the framesjs-starter code?
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 would like that but that would require a lot of work, so perhaps it is better to split it to another PR.
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.
agreed
feat: add next simple template fix: ignore some files when copying templates chore: remove fs-extra dep chore: add changeset remove comment chore: improve dependencies installation and run app in debugger by default chore: improve debugger hub comment chore: document installation chore: remove debug hub usage in case of production build chore: add two more examples based on starters chore: update to new dep
7e7b20e
to
55fab50
Compare
@stephancill it is ready. |
Change Summary
This PR adds
create-frames
package which can be used to init a project using a template.TODO
create-frames
cli toolnext
starter templatefs-extra
and use justnode:fs
Merge Checklist