-
-
Notifications
You must be signed in to change notification settings - Fork 125
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(create-waku): --example
option
#581
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
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.
https://github.com/dai-shi/waku/pull/581/files#diff-995f71dd25297bc098cf21c18a6770843422edd5863adc74f7b181ca7c803bcfR25
We would like to copy only examples/01_template
.
@himself65 Can you review please?
(Maybe, it's time for us to add some unit tests.)
Branch name or tag name is actually important because |
/** | ||
* this is a part of the response type for github "Get a repository" API | ||
* @see {@link https://docs.github.com/en/rest/repos/repos?apiVersion=2022-11-28#get-a-repository|GitHub REST API} | ||
*/ | ||
interface GetRepoInfo { | ||
/** A default branch of the repository */ | ||
default_branch: string; | ||
} |
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 take a part of the response type from github docs here
cc. @himself65
@dai-shi
This PR's implementation is only to download example from
I didn't understand this situation |
Let's say, we release v0.20.0. At that point, we can use Meanwhile, we will start merging PRs for v0.21.0, and examples will only work with the new (unreleased) version. If we run |
I got that situation. Still if you think it is nessasary to get the |
No, that's not the case.
Yes, it's necessary, but can be a follow-up PR. |
Please review this 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.
I didn't check the behavior though.
@@ -25,16 +25,18 @@ | |||
"start": "node ./dist/index.js", | |||
"dev": "ncc build ./src/index.ts -w -o ./dist/", | |||
"compile": "rm -rf template dist *.tsbuildinfo && pnpm run template && pnpm run build", | |||
"template": "cp -r ../../examples template/ && rm -rf template/*/dist && rm -rf template/*/node_modules && mv template/01_template/.gitignore template/01_template/gitignore", | |||
"template": "mkdir template && cp -r ../../examples/01_template template/01_template && rm -rf template/*/dist && rm -rf template/*/node_modules && mv template/01_template/.gitignore template/01_template/gitignore", |
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.
Maybe we can just cp -r ../../examples/01_template template
. You can consider it in the follow-up PRs.
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.
yeah i've tried to do that. but i've got it like below:
/template
/src
package.json
...
but i want to do this below:
/template
/01_template
because i consider that it will be added the extra 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.
okay, i thought the former is fine (single template). We don't know the future, but for now we have only one template.
return ( | ||
typeof err === 'object' && | ||
err !== null && | ||
typeof (err as { message?: unknown }).message === 'string' |
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.
Maybe this works?
typeof (err as { message?: unknown }).message === 'string' | |
typeof err.message === 'string' |
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.
okay i will try that next PR
|
||
// TODO automatically installing dependencies | ||
// 1. check packageManager | ||
// 2. and then install dependencies |
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.
can we use this? https://github.com/antfu/ni
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.
Oh thanks.
My plan is just below:
exec(`${packageManager} install`)
i don't know ni but if it is useful, let's use it.
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.
Yes that can work too. just wanted to drop a possibility.
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.
Pay special attention when choosing a dependency. we generally avoid adding new dependencies especially if they are new.
) { | ||
await pipeline( | ||
await downloadTarStream( | ||
`https://codeload.github.com/${username}/${name}/tar.gz/${branch}`, |
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 this downloading the whole waku repo for users?
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.
Yes, the only way to get examples of waku
is to download the whole waku repository.
Do you have any concerns or suggestions?
if you do, i would like to consider it
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.
Yea, it might be too much size to download for the user. If there's no other way then let's go with this. but it's better to find a way.
@dai-shi |
Basically, it feels like we are still in the middle of review.
out of curiosity, why do you prefer separating PRs? |
yeah i also agree with you. If you have any feedback, please let me know !
i prefer separating the works such as refactoring and other features. I just want to focus on the interests of this PR and i tend to separate the task into smaller tasks and prioritize them. |
Maybe we can create a branch, and send PRs there. Will let you know when it's appropriate. Meanwhile, send follow-up PRs to |
related #315
I wrote the prototype for
--example
option. I've referred to create-next-appI will be adding
automatically installing dependencies
in next PR.What features were added
also in this PR, i've drop --choose-example as @dai-shi said
#315 (reply in thread)
What I want to discuss
In create-next-app, they are dealing with the branch name or file path containing the slash. But I've thought this case is a bit rare one so I didn't implement this feature in create-waku. How about this case?