Skip to content
This repository has been archived by the owner on Mar 29, 2024. It is now read-only.

Migrate to React 15.5 #73

Merged
merged 3 commits into from
May 3, 2017
Merged

Conversation

ghost
Copy link

@ghost ghost commented May 1, 2017

Why?

  • React 15.5 was released, causing some changes in light of React Fiber coming
  • even though react is a peer dependency for this project (as you correctly state in Update to React 15.5  #67), some of the internal code for the library itself relies on old versions of React.
  • From the React blog:

we've extracted React.PropTypes and React.createClass into their own packages. Both are still accessible via the main React object, but using either will log a one-time deprecation warning to the console when in development mode.

Adding new warnings is not something we do lightly. Warnings in React are not mere suggestions — they are integral to our strategy of keeping as many people as possible on the latest version of React.

So while the warnings may cause frustration in the short-term, we believe prodding developers to migrate their codebases now prevents greater frustration in the future. Proactively fixing warnings ensures you are prepared for the next major release. If your app produces zero warnings in 15.5, it should continue to work in 16 without any changes.

What?

Prop-Types

CSS Transition Group

Enzyme

Question:

  • the one item i'm not 100% sure on is this line. I see the Webpack config is excluding the CSS Transition Group from including React in the build as described here. But I'm not sure the root property is the right way to do it.

What are your thoughts on this @brijeshb42 ?

@ghost ghost mentioned this pull request May 1, 2017
@codecov-io
Copy link

codecov-io commented May 1, 2017

Codecov Report

Merging #73 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #73      +/-   ##
==========================================
+ Coverage    86.2%   86.25%   +0.04%     
==========================================
  Files          15       15              
  Lines         319      320       +1     
  Branches       42       42              
==========================================
+ Hits          275      276       +1     
  Misses         44       44
Impacted Files Coverage Δ
src/components/entities/link.js 92.3% <100%> (+0.64%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f715d3b...fd6cff6. Read the comment docs.

@brijeshb42
Copy link
Owner

@baldwmic I build the files as UMD. So root property is used for exposing to window global. It should remain

['React', 'addons', 'CSSTransitionGroup']

because I just checked the latest build from https://unpkg.com/react@15.5.4/dist/react-with-addons.js and it has React.addons.CSSTransitionGroup.

@brijeshb42
Copy link
Owner

@baldwmic I will take a look this PR and merge. It may take some time.

@ghost ghost force-pushed the upgradeReact-15 branch from d190760 to a92dade Compare May 2, 2017 14:31
@ghost
Copy link
Author

ghost commented May 2, 2017

@brijeshb42 i've updated the PR per your comment about the build.

Thanks for taking a look and let me know if I've missed anything!

root: ['React', 'addons', 'CSSTransitionGroup'],
commonjs2: 'react-transition-group',
commonjs: 'react-transition-group',
amd: 'react-transition-group',
}
Copy link
Owner

Choose a reason for hiding this comment

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

Can you change this to

  {
      'react-transition-group/CSSTransitionGroup': {
        root: ['React', 'addons', 'CSSTransitionGroup'],
        commonjs2: 'react-transition-group/CSSTransitionGroup',
        commonjs: 'react-transition-group/CSSTransitionGroup',
        amd: 'react-transition-group/CSSTransitionGroup',
      }
    }

Otherwise the whole package is included in the build file which significantly increases the file size.

Everything else looks good and I'll merge once the changes are done.

Copy link
Author

Choose a reason for hiding this comment

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

@brijeshb42 I've updated the PR per your comment

Copy link
Author

Choose a reason for hiding this comment

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

definitely wouldn't want to increase build size unnecessarily

@ghost ghost force-pushed the upgradeReact-15 branch from a92dade to fd6cff6 Compare May 3, 2017 15:45
@ghost
Copy link
Author

ghost commented May 3, 2017

@brijeshb42 yup! just updated the PR

@brijeshb42 brijeshb42 merged commit fd3008c into brijeshb42:master May 3, 2017
@brijeshb42
Copy link
Owner

@baldwmic Just published v0.5.1 after merging your PR.

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

Successfully merging this pull request may close these issues.

2 participants