-
Notifications
You must be signed in to change notification settings - Fork 573
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 create-yoga-app for - feature #1387
base: main
Are you sure you want to change the base?
Conversation
|
I changed the base branch to v3, I will add further feedback later. |
"https://codeload.github.com/dotansimha/graphql-yoga/tar.gz/master" | ||
), | ||
tar.extract({ cwd: root, strip: 3 }, [ | ||
`graphql-yoga-master/examples/${name}`, |
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 am not sure whether we want to pull the examples from master as they might use different "functionality" than the one already released in a stable yoga version. 🤔
Maybe we can when we release a new yoga version create a git tag (e.g. stable-examples
) and it will fetch the examples from there.
Maybe we should even publish the examples to NPM and pull them from there?
I see right now that it is downloading the full tar and then extracting the example out of it 🤔 That seems like too much is downloaded to me.
appPath: string; | ||
packageManager: any; | ||
template?: string; | ||
templatePath: 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 guess we don't need two flags? what's the reason for having two?
|
||
interface YogaProps { | ||
appPath: string; | ||
packageManager: any; |
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.
should be typed?
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 as InstallArgs['packageManager']
?
import { install } from "./utils/install.js"; | ||
import os from "os"; | ||
|
||
interface YogaProps { |
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 you please add some comments explaining the flags here?
packageManager: any; | ||
template?: string; | ||
templatePath: string; | ||
typescript: 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 think we might not need --typescript
flags, as this is already part of the examples setting?
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.
Sure, will remove that.
"engines": { | ||
"node": ">=14" | ||
}, | ||
"devDependencies": { |
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 are usually pinning all devDepenencies
import path from "path"; | ||
|
||
export function isFolderEmpty(root: string, name: string): boolean { | ||
const validFiles = [ |
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.
Where did this list come from? should we document this somehow?
@@ -0,0 +1,25 @@ | |||
import { execaSync } from "execa"; | |||
export function getPackageManager(npm: boolean, pnpm: boolean) { |
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.
Should this be typed? InstallArgs['packageManager']
?
/** | ||
* Indicate whether there is an active Internet connection. | ||
*/ | ||
isOnline: boolean; |
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.
What's the reason for having isOnline
configured? what's the use-case?
/** | ||
* Indicate whether the given dependencies are devDependencies. | ||
*/ | ||
devDependencies?: boolean; |
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 assume this should be decided by the template?
9fbe8e7
to
54a67e6
Compare
332b565
to
f64f21f
Compare
This PR is for the feature #1354