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

Switch to React 16 #263

Merged
merged 16 commits into from
Jan 28, 2018
Merged

Conversation

forabi
Copy link
Contributor

@forabi forabi commented Jan 26, 2018

Currently has to be tested using NO_TYPE_CHECK=1 yarn dev

Currently has to be tested using `NO_TYPE_CHECK=1 yarn dev`
@forabi
Copy link
Contributor Author

forabi commented Jan 26, 2018

I got it to work, although there is a ton of TypeScript issues. Everything works, including server-side rendering.

Here is a comparison of the bundles with React 16 vs Preact:

React 16:
screencapture-127-0-0-1-8888-1516996801274

Preact:
screencapture-127-0-0-1-8888-1516996820257

@forabi
Copy link
Contributor Author

forabi commented Jan 26, 2018

I ran some benchmarks against a local production build, and to eliminate network variations, the app was built against a local API instance as well:

React 16:

❯ echo "GET http://localhost:3001/Tom_Hanks" | vegeta attack -duration 60s -rate 100 | tee results.bin | vegeta report
Requests      [total, rate]            6000, 100.02
Duration      [total, attack, wait]    1m0.000133875s, 59.989999871s, 10.134004ms
Latencies     [mean, 50, 95, 99, max]  16.617495ms, 12.18481ms, 35.455429ms, 48.908762ms, 79.393735ms
Bytes In      [total, mean]            149796000, 24966.00
Bytes Out     [total, mean]            0, 0.00
Success       [ratio]                  100.00%
Status Codes  [code:count]             200:6000  
Error Set:

Preact:

❯ echo "GET http://localhost:3001/Tom_Hanks" | vegeta attack -duration 60s -rate 100 | tee results.bin | vegeta report
Requests      [total, rate]            6000, 100.02
Duration      [total, attack, wait]    1m0.003478528s, 59.989999392s, 13.479136ms
Latencies     [mean, 50, 95, 99, max]  18.488392ms, 15.312506ms, 39.258539ms, 52.198818ms, 84.178161ms
Bytes In      [total, mean]            150114000, 25019.00
Bytes Out     [total, mean]            0, 0.00
Success       [ratio]                  100.00%
Status Codes  [code:count]             200:6000  
Error Set:

@wholesomedev
Copy link
Contributor

wholesomedev commented Jan 27, 2018

I'm not sure how to interpret the first image that shows the sizes. I guess it shows the relative difference? React doesn't seem that bad, except React DOM is pretty big 😄

As for the performance, the two are comparable from what I can tell with React having somewhat of an edge.

@forabi
Copy link
Contributor Author

forabi commented Jan 27, 2018

I'm not sure how to interpret the first image that shows the sizes. I guess it shows the relative difference? React doesn't seem that bad, except React DOM is pretty big smile

Yeah, this shows the relative module sizes using Webpack Bundle Analyzer (yarn build && yarn app/analyze). Here are the actual sizes:

  • vendor.js with React 16: 104KB gzipped, 420KB uncompressed (exceeds the enforced max size of 100KB).
  • vendor.js with Preact: 79KB gzipped, 338KB uncompressed.

The benchmarks above test the performance of SSR. It seems that React and Preact are comparable in performance and the difference is negligible.

@forabi
Copy link
Contributor Author

forabi commented Jan 27, 2018

It turns out that the TypeScript issues were due to multiple, incompatible versions of @types/react being installed in node_modules. Each @types/* package for a React library defines a different range for @types/react as a dependency:

❯ find ./node_modules -wholename "*/react/index.d.ts"
./node_modules/@types/react-router/node_modules/@types/react/index.d.ts
./node_modules/@types/react-router-redux/node_modules/@types/react/index.d.ts
./node_modules/@types/react-redux/node_modules/@types/react/index.d.ts
./node_modules/@types/react/index.d.ts
./node_modules/@types/react-router-dom/node_modules/@types/react/index.d.ts
./node_modules/@types/react-dom/node_modules/@types/react/index.d.ts

Removing all of these (except ./node_modules/@types/react/index.d.ts) fixed all the issues with typings.

This is why peerDependencies is a good idea 😄

Luckily, yarn has a feature to flatten a specific dependency so that only one version of it is installed. To do so, I added a resolutions field to package.json:

  "resolutions": {
    "@types/react": "^16.0.35"
  },

... and now everything works!

@forabi
Copy link
Contributor Author

forabi commented Jan 27, 2018

One thing I noticed is that Preact is much more permissive regarding SSR vs client markup mismatch issues. It tries to fix the mismatches and does so silently. React on the other hand throws an error and does not fix it and the page ends up broken.

@wholesomedev
Copy link
Contributor

Interesting.

Perhaps the libraries follow different philosophies with regard to failing loud vs failing silently? I wonder if React offers the option to be more tolerant of that issue. Though, if the React engineers think this is a significant issue that should be fixed instead of suppressed, they probably wouldn't offer the option to silence it.

@forabi
Copy link
Contributor Author

forabi commented Jan 27, 2018

This is relevant: facebook/react#10591

The styles not being patched up is expected. That's an intentional breaking change. The idea behind that is that most sites stabilize once they have a solid SSR infra in place and therefore these things don't happen, yet they would have to spend all that time validating unnecessarily when they know it's not going to change. So it's an optimization.

@forabi
Copy link
Contributor Author

forabi commented Jan 27, 2018

This one too: facebook/react#11064

In my understanding it is not expected to patch up the differences since doing that in production is too expensive and negates the performance benefits from the new approach.

@forabi forabi mentioned this pull request Jan 27, 2018
@forabi
Copy link
Contributor Author

forabi commented Jan 27, 2018

Oops! Please dismiss the review request. This still need some work. Because React does not like SSR/client mismatches, we have to come up with a reproducible random string for the collapsable ID. We'll need to use React Context API for this.

@forabi forabi removed the request for review from wholesomedev January 27, 2018 22:18
@forabi
Copy link
Contributor Author

forabi commented Jan 28, 2018

Using the current context API with TypeScript proved to be a little tricky. I ended up moving the responsibility of defining the collapsable ID to the consuming component. Hopefully, a new React version, with the new context API, gets released soon so that we can move back to auto-generated IDs.

Copy link
Contributor

@wholesomedev wholesomedev left a comment

Choose a reason for hiding this comment

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

Nice 👍

@forabi forabi merged commit 6f0d5a0 into hollowverse-archive:master Jan 28, 2018
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.

2 participants