Skip to content
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

Upgrade dev dependencies #1533

Merged
merged 5 commits into from
Feb 9, 2019
Merged

Upgrade dev dependencies #1533

merged 5 commits into from
Feb 9, 2019

Conversation

li-kai
Copy link
Member

@li-kai li-kai commented Feb 9, 2019

  1. Upgrades babel & jest
  2. Improve jest performance
  3. Improve babel and js performance

@li-kai li-kai requested a review from a team February 9, 2019 03:35
@mods-bot
Copy link

mods-bot commented Feb 9, 2019

Deploy preview for nusmods ready!

Built with commit 853980b

https://deploy-preview-1533--nusmods.netlify.com

www/babel.config.js Show resolved Hide resolved
www/babel.config.js Outdated Show resolved Hide resolved
@ZhangYiJiang
Copy link
Member

Code coverage somehow increased 16%. Either you broke the code coverage path config or you fixed it. Can you check which one it is? :P

@ZhangYiJiang
Copy link
Member

CI seem to be slightly faster, which is nice, and bundle size is smaller by 4kb gzipped, which is a small win

www/package.json Outdated Show resolved Hide resolved
@li-kai
Copy link
Member Author

li-kai commented Feb 9, 2019

@ZhangYiJiang It was broken because it wasn't covering all the files without a test file beside.

However, test coverage still went up, because we ignore test-utils instead of test_utils now 💯

@li-kai li-kai force-pushed the kli/babel branch 2 times, most recently from 1364a3b to 1317461 Compare February 9, 2019 05:52
Copy link
Member

@ZhangYiJiang ZhangYiJiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly minor stuff. To be honest I'm not entirely sure if replacing the static config object with this is a good idea, but it's relatively minor either way.

www/jest.config.js Show resolved Hide resolved
],
snapshotSerializers: ['<rootDir>/node_modules/enzyme-to-json/serializer'],
collectCoverageFrom: ['**/*.{js,jsx}'],
coveragePathIgnorePatterns: ['/node_modules/', '<rootDir>/src/js/(?:test-utils|e2e)'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the ?: is unnecessary. It makes it a non-capturing group, which captures intent better, but it makes it a bit harder to read. I can go either way 🤷‍♂️

@li-kai li-kai merged commit 3c8b36e into master Feb 9, 2019
@li-kai li-kai deleted the kli/babel branch February 9, 2019 17:49
dvrylc pushed a commit to dvrylc/nusmods that referenced this pull request Feb 16, 2019
* Upgrade babel config

* Update jest config

* Improve test performance

* Align babel config with create-react-app

* Fix various config flows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants