-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Update babel to use env
and react-app
as presets.
#976
Conversation
01a2196
to
6090cc0
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.
This looks good to me -- thanks for adding the web
script! Nice to see all of that logic in a single place. I just had some small suggestions below.
bin/web
Outdated
|
||
DEV_PORT=8080 | ||
export NODE_ENV=${NODE_ENV:=development} | ||
PUBLIC_API_SOURCE=k8s |
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 don't see the PUBLIC_API_SOURCE
var used anywhere. Maybe remove it?
bin/web
Outdated
function err { local x=$? ; msg "$*" ; return $(( $x == 0 ? 1 : $x )) ;} | ||
function out { printf '%s\n' "$*" ;} | ||
|
||
if [[ ${1:-} ]] && declare -F | cut -d' ' -f3 | fgrep -qx -- "${1:-}" |
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 prefer grep -F
here, since fgrep
may not be present on some systems.
bin/web
Outdated
esac | ||
done | ||
|
||
nc -z localhost 8085 || port-forward & |
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 is super handy for the dev
target. I think it would also be useful to add it to the run
target below.
web/Dockerfile
Outdated
# node dependencies | ||
RUN $HOME/.yarn/bin/yarn install --pure-lockfile | ||
RUN $ROOT/bin/web setup --pure-lockfile | ||
RUN ls $PACKAGEDIR/node_modules | grep babel |
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 looks like a stray debug RUN command. Maybe we should remove it?
@@ -60,9 +60,9 @@ jobs: | |||
- curl -o- -L https://yarnpkg.com/install.sh | bash -s -- --version 1.3.2 | |||
- export PATH="$HOME/.yarn/bin:$PATH" | |||
install: | |||
- cd web/app && yarn && yarn webpack | |||
- ./bin/web build |
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.
As far as I can tell, the CI failure happening on this branch stems from the fact that we're no longer running yarn
without any arguments -- we're just running yarn webpack
, which is failing. I think you can fix this with:
install:
- ./bin/web setup
- ./bin/web build
6090cc0
to
4f21c5e
Compare
@klingerf ahhhha! Thank you! Should be all updated now (and it looks like the tests are passing again). |
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, thanks for making those updates. I just had one more question about dev dependencies below.
web/app/package.json
Outdated
"babel-preset-es2015": "^6.24.1", | ||
"babel-plugin-syntax-object-rest-spread": "^6.13.0", | ||
"babel-plugin-transform-object-rest-spread": "^6.26.0", | ||
"babel-preset-env": "^1.7.0", | ||
"babel-preset-react": "^6.24.1", |
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 we remove the "babel-preset-react" dev dependency now that we're no longer using the "react" preset in .babelrc?
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 can strip these down quite a bit more actually ....
- Switched from `es2015` to `env` for the default preset. This is the recommended preset and allows us to track the latest and greatest moving forward. - Added `react-app` as a preset. We get class properties (and thus => for context) as well as the current recommended settings for react apps. - Created a `web` script that provides functions for common tasks. `react-app` requires that BABEL_ENV/NODE_ENV is set and this guarantees it. - Updated the web dockerfile to set NODE_ENV correctly and use `bin/web`. - Moved the babel related modules over to devDependencies.
4f21c5e
to
0713bf0
Compare
es2015
toenv
for the default preset. This is the recommended preset and allows us to track the latest and greatest moving forward.react-app
as a preset. We get class properties (and thus => for context) as well as the current recommended settings for react apps.web
script that provides functions for common tasks.react-app
requires that BABEL_ENV/NODE_ENV is set and this guarantees it.bin/web
.