Skip to content

feat(dependencies): update to React 16 and Prettier 1.7#55

Merged
priley86 merged 1 commit intopatternfly:masterfrom
priley86:react-16
Oct 3, 2017
Merged

feat(dependencies): update to React 16 and Prettier 1.7#55
priley86 merged 1 commit intopatternfly:masterfrom
priley86:react-16

Conversation

@priley86
Copy link
Member

@priley86 priley86 commented Oct 1, 2017

What:
updates React/React DOM dependency, React Bootstrap dependency, and Storybook dependency for React 16

@@ -0,0 +1,7 @@
{
// Place your settings in this file to overwrite default and user settings.
Copy link
Member

Choose a reason for hiding this comment

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

comments are not supported in json?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's weird that VSCode put these in mine by default. I can remove them..

package.json Outdated
"patternfly": "^3.27.4",
"prop-types": "^15.5.10",
"react": "^15.5.0"
"react": "^15.5.0 || ^16.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

should we bump to 15.6.2? it will at least show deprecation warnings but will work?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am OK with either (as long as we can support downstreams). I will be trying to update all of my projects to 16 soon. Any thoughts on this version @jtomasek ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@ohadlevy I see why you are saying 15.6.2 now...from the release notes:
https://facebook.github.io/react/blog/2017/09/26/react-v16.0.html

We've been serving React 16 to Facebook and Messenger.com users since earlier this year, and we released several beta and release candidate versions to flush out additional issues. With minor exceptions, if your app runs in 15.6 without any warnings, it should work in 16.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated to 15.6.2 @ohadlevy

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would not be so strict in terms of bumping peerDependencies as it forces the consuming apps to constantly update too. It definitely makes sense in case of React 16, although I'd keep 15.5.0 for now. TripleO UI is currently updating to 15.6.2 to get under MIT license. We won't be able to update for some time as we're currently hitting a few deprecation warnings on dependencies (Mostly Formsy-react which we're moving from towards redux-form).

Copy link

@waldenraines waldenraines left a comment

Choose a reason for hiding this comment

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

This all looks fine to me except for changes requested from other reviews so I'll mark this as 'request changes'.

@priley86 priley86 force-pushed the react-16 branch 2 times, most recently from caba538 to 960e5a2 Compare October 2, 2017 18:59
@priley86
Copy link
Member Author

priley86 commented Oct 2, 2017

@ohadlevy @waldenraines @jtomasek one other thing i've noticed after looking at this again. I seem to be seeing subtle changes in the way Prettier is formatting the code from minor release to minor release, therefore I am locking the version at 1.7.3 for now. I would rather avoid the hassle of seeing Prettier changes conflating other PRs with changes, and only update Prettier on the occasion we want to going forward.

@jtomasek
Copy link
Collaborator

jtomasek commented Oct 3, 2017

@priley86 locking the Prettier version should not be needed as it is locked in package-lock.yaml If some PR introduces unrelated prettier formating changes, it is caused by installing pf-react dependencies with older npm version than npm5

"lint": "eslint --max-warnings 0 src storybook",
"prettier":
"prettier --write --single-quote --no-semi '{src,storybook}/**/*.js'",
"prettier": "prettier --write --single-quote --no-semi '{src,storybook}/**/*.js'",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prettier now supports prettierrc config file, so we can extract some of these CLI options into it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jtomasek we can add this in a future PR along with prettier-stylelint if you want. I have added prettierignore to prevent conflicts in package.json

Copy link
Member Author

Choose a reason for hiding this comment

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

fwiw, i still <3 Prettier... i did some testing recently and found that it resolves over 40% of the Standard JS Lint rules automatically ;)

https://gist.github.com/priley86/78093a1f37b39242096c13ddfbe3f701

updates React/React DOM, React Bootstrap, and Storybook dependencies for React 16
@priley86
Copy link
Member Author

priley86 commented Oct 3, 2017

@jtomasek reverted Prettier change for now. We just need to ensure people are using npm5/node8 or we will possibly see Prettier changes conflated.

Also noted that Storybook 3.1.x is not compatible with React 16, however 3.2.x is (thus the updates there).

@priley86 priley86 merged commit 8d3110f into patternfly:master Oct 3, 2017
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