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

Maintenance: JS dependency updates #2759

Merged
merged 26 commits into from Feb 6, 2020
Merged

Maintenance: JS dependency updates #2759

merged 26 commits into from Feb 6, 2020

Conversation

kevinrobinson
Copy link
Contributor

@kevinrobinson kevinrobinson commented Feb 6, 2020

Who is this PR for?

developers, prompted by #2758

What problem does this PR fix?

#2758 Updated some JS dependencies, prompted by a false-positive vulnerability alert. It passed CI but the deploy failed. This led me to look more at what was going on, and how this could happen without failing the build. In the process, I updated the build process and many of the JS dependencies.

impact on build size

What does this PR do?

Yarn

  • Resolved all warnings
  • Added yarn-checks-cli, mostly for use when mucking with dependencies locally, but added it to CI as well.
  • note: docs say that yarn check is deprecated and full of false-positives

Webpack

  • Migrate to v4, upgraded all plugins
  • UglifyJS is abandoned, migrated to Terser per Webpack's recommendation
  • I looked at migrating to v5, but it's still in beta and not all plugins are ready, so backed that out.
  • Changed local development devtool sourcemap strategy, since it regressed the sourcemap quality. This is a bit slower but fine for now, and easy to change in isolation after.

Polyfills

  • Migrate to use react-app-polyfill from create-react-app, instead of our own.
  • We had some old measurement code we haven't been using, and it relied on performance.now. Remove those files and the pasted performance.now polyfill.

eslint

  • Added eslint-plugin-compat to catch when our code uses JS APIs that aren't supported in our target browsers. This also flagged Response and URLSearchParams, which were only used as test so set to ignore.
  • eslint-plugin-compat reads from .browserlistrc, and then reads .eslintrc settings to see what polyfills we expect to be present. This meants the values in .eslintrc need to stay in sync with polyfills.js, so added comments in both places about that.
  • Removed es6: true flag, since this includes JS APIs not just language features. From eslint's perspective, this is best enforced by eslint-plugin-compat and the settings for globals and polyfills.

Testing

  • Updated all packages related to jest tests
  • Migrated from using Enzyme'swrapper.html() with mount tests. Some tests failed after the upgrade, and investigated it turned out it was from lag between the DOM updating and the Enzyme #html method reflecting that (the Enzyme docs had a method for manually synchronizing). Instead, just avoid this and check the DOM directly.

Storybook

  • Added storybook-cli to CI, to do a smoke test on building and loading stories.
  • Updated task so browser stops opening when running Storybook

Other notes

  • query-string@6 isn't ES5 compatible, so kept it at 5 per those docs
  • I tested this locally in IE11, and will manually verify the demo deploy on IE11 before a full deploy
  • Main JS bundle size drops by about 10%
  • yarn outdated looks like this now, 16 dependencies down from 60 prior to this PR:

Screen Shot 2020-02-06 at 1 47 18 PM

Not included

This doesn't updated any major versions of significant libraries (eg, moment, d3, highcharts).

I did look at upgrading React libs, but the React dependencies are also behind (eg, react-select@1 leads to many warnings on lifecycle methods in React 16.9, react-virtualized appears to require core-js@2). So I cut that out. To move that forward, it would be better to upgrade those dependencies first (or replace them, react-autocomplete appears abandoned), and then upgrade the lib. Our code is straightforward for 16.9 at least.

Checklists

Which features or pages does this PR touch?

  • Core, JS

Does this PR use tests to help verify we can deploy these changes quickly and confidently?

  • Included specs for changes
  • Manual testing made more sense here

@kevinrobinson
Copy link
Contributor Author

@kevinrobinson kevinrobinson commented Feb 6, 2020

👍 in demo 🚢

@kevinrobinson kevinrobinson merged commit 07b784d into master Feb 6, 2020
1 check passed
@kevinrobinson kevinrobinson deleted the patch/upgrading-js-deps branch Feb 6, 2020
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.

None yet

1 participant