added npm scripts to simplify local setup and running dev servers #44
Conversation
vladlos
requested review from
j-low,
laurenbarker and
nathanstilwell
as code owners
| @@ -3,5 +3,9 @@ | |||
| "private": true, | |||
| "devDependencies": { | |||
| "lerna": "^3.20.2" | |||
| }, | |||
| "scripts": { | |||
| "init": "yarn && lerna bootstrap --force-local && lerna link", | |||
There was a problem hiding this comment.
lerna bootstrap should install dependencies and symlink all of the packages so yarn and lerna link shouldn't be necessary.
There was a problem hiding this comment.
Why --force-local? I haven't worked with lerna very much so I'm not sure if this is common but I would think our versions should match when running init.
When passed, this flag causes the bootstrap command to always symlink local dependencies regardless of matching version range.
There was a problem hiding this comment.
first yarn is needed to install top level packages. lerna call will fail without it, except case when you have installed lerna -g.
--force-local is not mandatory but due to ^ version notation in packages we can catch an edge case when newer version of package being published to npm, but not merged in main brunch. or you in the middle of development of some feature and do not want or forget to rebase and need to run init. I am not sure in how realistic this scenarios. With --force-local you avoid that external package being fetched and\or cached somewhere, and you end up with making changes, and do not seeing them while hot-reload.
additional lerna link is also not mandatory but, at least on my machine I encountered cases when I need it additionally after bootstrap. I don't know due to what it happening (npm caches, wrong moon phase?) but adding this as after bootstrap works pretty stable.
There was a problem hiding this comment.
Thanks for the explanation! Would you mind adding then instructions to get things started and the explanation of what the commands do to the README?
packages/ui-components/package.json
Outdated
| @@ -13,7 +13,8 @@ | |||
| "clean": "rm -rf ./dist/*", | |||
| "lint": "eslint ./src/**/*.{tsx,ts,js} --format=codeframe", | |||
| "test": "jest --watch", | |||
| "watch": "webpack" | |||
| "watch": "webpack", | |||
There was a problem hiding this comment.
I know you didn't add this line but should it be webpack --watch?
There was a problem hiding this comment.
Maybe also unify with the icons package and change the name to build:watch since start will simplify it anyway.
There was a problem hiding this comment.
webpack command in ui-components starts webpack dev server. and build:watch seems to actually create dist files on every change. So I would not rename them as this commands already sorta correctly named to be self explanatory.
There was a problem hiding this comment.
We don't have webpack-dev-server installed but it looks like watch is set to true in the webpack config https://github.com/cockroachdb/ui/blob/master/packages/ui-components/webpack.config.js#L7
There was a problem hiding this comment.
I think removing watch from the config and then updating "watch": "webpack" to be "build:watch": "webpack --watch" would be the most clear here. It's building when files change like the ui-components repo. Does that make sense to you @elkmaster?
packages/ui-components/src/index.js
Outdated
| @@ -1 +1,2 @@ | |||
| export { Badge } from "./Badge"; | |||
| export { Tooltip } from "./Tooltip"; | |||
There was a problem hiding this comment.
Looks like the tooltip import snuck in here
ui: DB Console branding refresh
UI
Provided 2 new npm scripts on top level:
npm run init- will install all node modules and run needed lerna commands for local setupnpm start- will start startwatchdev servers for all packages inside one terminal window. no need to cd manually and having multiple terminals.Checklist
I have written or updated test for the changes I madeI have added or updated Storybook if appropriate for my changes