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 frontend dependencies (part 1) #12468

Closed
wants to merge 22 commits into from
Closed

Conversation

tommyip
Copy link
Member

@tommyip tommyip commented Jun 2, 2019

Upgrade low-impact (patch and minor level updates) frontend dependencies.
The remaining packages that need to be updated in another PR are either:

  1. New major version
  2. jQuery (deprecated positional selector in favor of positional methods, eg :first -> .first())
  3. to-markdown (renamed to Turndown, and API changes)
  4. css-hot-loader (mini-css-extract-plugin can now do hot-module-replacement so this is not needed anymore)
  5. emoji-datasource-(google|twitter) (our current codebase have some inconsistent reference to the old EmojiOne source and the new emoji-datasource package removed EmojiOne in v4.1.0, need to investigate into this more)

@timabbott should each commit bump the provision version or just the final one?

Testing Plan:
Read all the change logs to check if there are any breaking changes / manual testing

GIFs or Screenshots:

@tommyip
Copy link
Member Author

tommyip commented Jun 2, 2019

Interesting, I can't reproduce the tools/webpack.config.ts(4,35): error TS2307: Cannot find module 'webpack-dev-server'. error when running ./tools/minify-js locally.

@priyank-p
Copy link
Member

should each commit bump the provision version or just the final one?

Usually the final commit should bump the provision version. Since when all the dependencies upgrade commit are merged we only have to bump the version once so others do tools/provision.

@@ -1,6 +1,7 @@
import { resolve } from 'path';
import * as BundleTracker from 'webpack-bundle-tracker';
import * as webpack from 'webpack';
import * as webpackDevServer from 'webpack-dev-server';
Copy link
Member

Choose a reason for hiding this comment

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

This is what causes the failure in travis ci because webpack-dev-server is a marked as a dev dependency hence not installed during the production install workflow. You can't reproduce this failure locally since tools/provision installs webpack-dev-server.

@@ -16,9 +17,13 @@ function getHotCSS(bundle: any[], isProd: boolean): any[] {
].concat(bundle);
}

export default (env?: string): webpack.Configuration => {
interface Configuration extends webpack.Configuration {
devServer?: webpackDevServer.Configuration;
Copy link
Member

Choose a reason for hiding this comment

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

Does webpack types really does not have types for devServer config? I'd expect them to have that since it part of webpack configuration.

Copy link
Member

Choose a reason for hiding this comment

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

There are typescript types avalible for webpack-dev-server at @types/webpack-dev-server. I think you can use them instead of having to import webpack-dev-server since that's and extra dependency that we don't want to install in production.

Copy link
Member Author

Choose a reason for hiding this comment

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

The devServer member was moved out of webpack into webpack-dev-server in one of the new versions.

@tommyip
Copy link
Member Author

tommyip commented Jun 2, 2019

@priyank-p cool thanks for the help!

@tommyip tommyip force-pushed the upgrade-deps branch 2 times, most recently from 69ef362 to 36a0336 Compare June 2, 2019 16:08
@timabbott
Copy link
Member

Looks like it's failing the frontend linter in CI.

For the emoji thing, @HarshitOnGitHub can probably provide the backstory.

@priyank-p
Copy link
Member

(Just want to mention that the webpack upgrade along with webpack-dev-server fixed the bug we face before.)

@timabbott
Copy link
Member

This is great, merged, thanks @tommyip and @priyank-p! Aside from continuing work on upgrading the rest, can you do a follow-up test of using testing responsive:true on our /stats charts? https://zulip.readthedocs.io/en/latest/subsystems/analytics.html has docs on how to test that page using the "analytics" realm. Or open an issue.

Again, huge thanks for doing this upgrade!

@timabbott timabbott closed this Jun 3, 2019
@tommyip tommyip deleted the upgrade-deps branch June 4, 2019 01:52
@tommyip
Copy link
Member Author

tommyip commented Jun 4, 2019

Yeah I will work on that. The stats page need some restructuring to allow the responsive option to work properly since we are currently fixing the charts to 750px wide.

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

Successfully merging this pull request may close these issues.

4 participants