-
Notifications
You must be signed in to change notification settings - Fork 32
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
Feature: Basic CLI #24
Conversation
0c3d144
to
dc8b897
Compare
58c8aa4
to
9bad6f1
Compare
140e5a7
to
074e8de
Compare
33841c6
to
e39b53b
Compare
Codecov Report
@@ Coverage Diff @@
## chore/init-ci #24 +/- ##
=================================================
- Coverage 91.60% 91.28% -0.32%
=================================================
Files 188 199 +11
Lines 2383 2594 +211
Branches 271 277 +6
=================================================
+ Hits 2183 2368 +185
- Misses 160 181 +21
- Partials 40 45 +5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
ed9a2d1
to
f34c021
Compare
e39b53b
to
c887048
Compare
- write some initial files for demo - fix in-memory code loader name
c887048
to
128fdcd
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.
Thanks for adding cli package, LGTM 👍
@@ -0,0 +1,13 @@ | |||
import { execSync } from 'child_process'; | |||
|
|||
if (process.env.READY_FOR_PUBLISH !== '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.
Great for adding READY_FOR_PUBLISH
to prevent unnecessary enter publish
👍
"assets": ["packages/build/*.md"] | ||
} | ||
"assets": ["packages/build/*.md"], | ||
"buildableProjectDepsInPackageJsonType": "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.
Is the property buildableProjectDepsInPackageJsonType
could the nx automatically add dependencies and buildable libraries so that we could publish a library with all dependencies ? (nrwl/nx#4620)
if it is true, maybe you could add the comment to let others know the purpose so that we could remind it to add when we create new nx packages and would like to publish in the future?
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://nx.dev/packages/js/executors/tsc
The property updateBuildableProjectDepsInPackageJson
control whether to add dependencies to package.json or not, its default value is true.
The setting here enforces the generator put these dependencies to "dependencies" but not "peerDependencies", I'll add some comments to tell the reason
Update: We can't add comments in JSON file 🤣
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.
Thanks @oscar60310 for answering my question, it was really helpful for me to understand its importance in the project.
That's fine. 😄 Still thanks for sharing the message, if there, I will memo it in my note.
packages/cli/src/commands/build.ts
Outdated
config: string; | ||
} | ||
|
||
const defaultConfig: BuildCommandOptions = { |
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.
NIT: maybe you could rename to defaultOptions
because the options contain config.
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, I'll update them.
import * as jsYAML from 'js-yaml'; | ||
import { promises as fs } from 'fs'; | ||
import * as path from 'path'; | ||
import * as ora from 'ora'; |
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.
Wow, the cool package for showing running/loading style 😲
packages/cli/src/commands/serve.ts
Outdated
port: number; | ||
} | ||
|
||
const defaultConfig: ServeCommandOptions = { |
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.
NIT: Same suggestion rename like above build
- add descriptions, license, repo ... for each package - use dependencies instead of peerdependencies of most dependency - set peerdependencise for Vulcan imports
128fdcd
to
2f75878
Compare
Hi @kokokuo All issues have been fixed besides the project.json comments~ thanks! |
Description
Add CLI package and implement basic commands.
Testing
Use command
nx install cli
to build and install Vulcan from local, then runvulcan init
to create new project.