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

Convert components to functions #6474

Closed
wants to merge 6 commits into from
Closed

Convert components to functions #6474

wants to merge 6 commits into from

Conversation

TrySound
Copy link
Contributor

@TrySound TrySound commented Nov 13, 2018

In this diff I converted all (except Router and Route) class components to functions with Lifecycle util.

As a result react-router-dom bundle is reduced with 1kB and the bundle with the following entry point is reduced from 16.5kB to 13.9kB.

import { Route } from 'react-router-dom';
console.log(Route);

Eye analyses of prettified result shows the following artifacts

  • webpack runtime
  • path-to-regexp
  • create-react-context
  • react-is
  • Router.js
  • matchPath.js
  • Route.js
  • RouterContext.js

So Route.js and Router.js are components which always appear in user code. There's no reason to treeshake them. All other modules are their dependencies.

v5 optimisation strategy

  • remove create-react-context
  • try to convert path-to-regexp to esm
  • remove React.createContext checks in Route and Router
  • convert Route and Router to function components

This should make project fully treeshakable and quite small.

What do you think guys? Will this work for you?

/cc @mjackson @billyjanitsch

@mjackson
Copy link
Member

Thanks for your work here, @TrySound. Can you please help me understand why webpack is not able to tree-shake JavaScript classes? I've been doing some research this morning, but haven't fully figured it out yet.

@TrySound
Copy link
Contributor Author

TrySound commented Nov 14, 2018

I don't know.

Rollup is not able to treeshake them because of transpiled class iife. It's not easy to infer class from it.

Terser could eliminate it with pure annotation, but seems like it doesn't look deep and removes only classes without dependencies. Dependants are left untouched.

So the solution is to go with actual treeshakability (not terser's dead code elimination) and rely on bundlers in it. But this is possible only with functions.

Burn classes everywhere! :)

@Andarist
Copy link
Contributor

From what I know - classes can be tree shaken just fine, the problem lies in transpiled classes WITH static properties. Without statics properties those transpiled IIFEs can be removed thanks to #__PURE__ annotations added by babel@7

I've written an issue about it once and wanted to make a blog post out of it later but never got time to do it, it's still there though bvaughn/react-virtualized#889 . So if you are interested in reading a little bit more about this you might take a look at my writeup there, it might be a little bit chaotic though - so sorry about that in advance 😅

@TrySound
Copy link
Contributor Author

@Andarist I found that pure annotations are not considered deeply. Dependants (event with pure annotations) are left in minified bundle.

@Andarist
Copy link
Contributor

Could u showcase the problem with an example? I haven't noticed any serious deopts with them.

@eps1lon
Copy link
Contributor

eps1lon commented Nov 24, 2018

Since the semver impact was not discussed yet I just wanted to add the converting class components to function components is breaking WRT ref if those function components are not wrapped in React.forwardRef and unless you explicitly documented that your components cannot hold refs.

Disregard this if the change is only aimed at v5 for which full forwardRef support is planned anyway as far as I understood #6056 (comment).

@mjackson
Copy link
Member

AFAICT @Andarist is correct; webpack is able to eliminate dead code just fine as long as we use the #__PURE__ annotations added in Babel 7, which we do. I did a bunch of research here and came to the same conclusion: the problem is adding static properties to classes.

I believe I removed the last of the static properties in 9d278b4 when I removed support for the old context API. We always told people to not use our context API directly, so I do not consider this a breaking change.

Anyway, I don't think we need this anymore. But thanks for the PR.

@mjackson mjackson closed this Feb 22, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Apr 23, 2019
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.

4 participants