-
Notifications
You must be signed in to change notification settings - Fork 64
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-web-scripts-library): implement cli and script #20
Conversation
5bb650e
to
3a1f15f
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.
nice!
### With `yarn create` | ||
|
||
```sh | ||
yarn create @spotify/web-scripts-library my-cool-library |
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.
yarn somehow knows where to inject the create
?
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.
yep! yarnpkg/yarn#6239
|
||
```sh | ||
npm i -g @spotify/web-scripts-create-library | ||
web-scripts-create-library my-cool-library |
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'd prefer if we avoided promoting a global install, they tend to lead to nothing but pain!
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.
it's a fact. happy to remove it, people can figure it out on their own if they really need 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.
Some small comments, but looks really good!
"extends": "@spotify/web-scripts/config/tsconfig.json", | ||
"include": ["src"], | ||
"compilerOptions": { | ||
"incremental": true, |
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.
This should probably be the default in the base config!
"incremental": true, | ||
"isolatedModules": false, | ||
"noEmit": true, | ||
"declaration": false, |
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.
for both noEmit and declaration, doesn't web-scripts handle these flags? I don't think we should imply they need to be managed by repeating them 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.
huh, I have no clue why I have these differences like this. Will clear them out.
Why: in lieu of an
init
script like Storybook does, this simpler path to create a scaffolded app using https://github.com/spotify/web-scripts-library-template in the style of create-next-app or create-react-app. This also means you don't need to ship any of the scaffolding deps (such as fs-extra) with web-scripts.Fixes #11