Automattic / notifications-panel

Core notifications panel for WordPress.com notifications
0 stars 1 forks source link

Upgrade notifications-panel to React 16 #191

Closed samouri closed 6 years ago

samouri commented 6 years ago

While trying to upgrade Calypso to React 16, the first thing that blew up at runtime was notifications-panel. That was because of React.PropTypes. I figured while here I might as well upgrade notifications-panel to 16 (will also ensure I don't miss anything).

Changelist:

  1. changes all React.PropTypes --> the one found in the separate prop-types package using the official prop-types codemod.
  2. changes all React.createClass calls to either createReactClass or es6 classes using the official class codemod.
  3. update react-redux, react, and react-dom in package.json.
  4. profit

To Test

  1. rm -rf node_modules
  2. npm install
  3. npm start
  4. ensure notifications-panel boots as expected and do a short smoke test.
  5. also ensure the new bundle sizes are much smaller than the old ones
samouri commented 6 years ago

This is strange, i'm seeing the build.min.js size go up after the upgrade. before: 2.6M after: 2.7M

...investigating

samouri commented 6 years ago

The numbers above were from a regular build. Here are new numbers from npm run build:prod: before: 908 kB after: 909 kB

I'm confused that the build didn't get smaller :/

samouri commented 6 years ago

I tested in FF nightly and Chrome (on frontend through proxy) and both LGTM.

I'm 🚢 ing

gwwar commented 6 years ago

@kwight we may need to publish 2 versions of notification-panel until Calypso is on React 16. Maybe this can land in a react16 branch, and we can merge fixes into both master and the react16 branch.

The feature branch will have an npm version of 2.0.0, with our master on 1.3.0. Any fixes that make it into master should also be merged into that feature branch.

dmsnell commented 6 years ago

publish 2 versions of notification-panel until Calypso is on React 16.

@gwwar is there anything that conflicts with React 15 in there? is it just createReactClass() which adds to the bundle build size?

gwwar commented 6 years ago

Just that Calypso isn't on 16 yet, we'll be juggling 2 branches in the meantime. I don't think there should be much overhead.