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

Migrate to React 16 #655

Merged
merged 6 commits into from
May 10, 2018
Merged

Migrate to React 16 #655

merged 6 commits into from
May 10, 2018

Conversation

gedeagas
Copy link
Contributor

@gedeagas gedeagas commented May 9, 2018

Motivation

Sadly Docusaurus is still using the old React 15 :(. With many major improvement and features that react v16 offers, I think this is the right time to update.

Fixes #539

Have you read the [Contributing Guidelines on pull requests]

Yes

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label May 9, 2018
@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-preview ready!

Built with commit 0758067

https://deploy-preview-655--docusaurus-preview.netlify.com

@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented May 9, 2018

Deploy preview for docusaurus-preview ready!

Built with commit 4a0f491

https://deploy-preview-655--docusaurus-preview.netlify.com

@yangshun
Copy link
Contributor

yangshun commented May 9, 2018

Thanks for doing this @gedeagas! I wanted to wait till we had some tests in place before upgrading to React 16, but I guess it if builds it should be fine 😄

Just to be clear, Docusaurus generates static output in the build step using React and is more of a website rather than a web app, so we don't really benefit too much from the perf improvements in React 16.

I'll do a little more extensive testing before approving. Is this PR still WIP?

@JoelMarcey
Copy link
Contributor

Wow - that is all it takes to upgrade? I thought it would have taken a bit more 😄

* Separate users data from code

* Add WarriorJS to Docusaurus users
@gedeagas
Copy link
Contributor Author

gedeagas commented May 9, 2018

Hi @yangshun I know, we use react renderToStaticMarkup right? I am planning to change the function to renderToStaticNodeStream but I guess it outputs the same thing and we don't need to change it.

Maybe any of the guys here want to use fragments or other feature that would be available in the next release ?. I know for our use case upgrading to v16 doesn't offer us many features or improvements.

Btw I didn't do much research on this but isn't renderToStaticMarkup on v16 is a bit faster than on v15?

@gedeagas
Copy link
Contributor Author

gedeagas commented May 9, 2018

Hi, @JoelMarcey that's what I am amazed at 😀.

I used to upgrade my company code base from v15 to v16 and it is a lot of work. We need to use react-codemod to help update React APIs and we also manually do search and replace to change any deprecated API to the new one.

I guess like @yangshun says because we use renderToStaticMarkup there is not much that changed from v15 to v16. Also, we only have one component that uses propTypes 😅so i only need to change that one file.

After I upgrade the react version and change that one component I run the test and all test came out as passed and the site runs beautifully. Imagine my surprise 😮, because I do expect a lot of work from this

@gedeagas
Copy link
Contributor Author

gedeagas commented May 9, 2018

@yangshun please test it, That is the reason why I tag this as WIP. If you guys find something fishy or wrong I will be happy to work on it.

ahmadalfy and others added 3 commits May 9, 2018 16:59
* Introduce imagemin for compressing images

* Replace original images with the optimized ones

* Add imagemin-svgo to dependencies

* Remove console statement, replace let with const

* Replace let with const

* Add --skip-image-compression

* Run Prettier
@yangshun yangshun changed the title [WIP] Migrating to React 16 Migrate to React 16 May 10, 2018
@yangshun yangshun merged commit 25cf8bb into facebook:master May 10, 2018
@yangshun
Copy link
Contributor

Tested it locally and it worked great. Thanks for the help!

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

Successfully merging this pull request may close these issues.

7 participants