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

Adds Babel plugin babel-plugin-optimize-react #6219

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
@trueadm
Copy link

trueadm commented Jan 17, 2019

This PR adds a Babel 7 plugin that aims to optimize certain React patterns that aren't as optimized as they might be. For example, with this plugin the following output is optimized as shown:

// Original
var _useState = Object(react__WEBPACK_IMPORTED_MODULE_1_["useState"])(Math.random()),
    _State2 = Object(_Users_gaearon_p_create_rreact_app_node_modules_babel_runtime_helpers_esm_sliceToArray_WEBPACK_IMPORTED_MODULE_0__["default"])(_useState, 1),
    value = _useState2[0];
    
// With this plugin
var useState = react__WEBPACK_IMPORTED_MODULE_1_.useState;
var __ref__0 = useState(Math.random());
var value = __ref__0[0];

Named imports for React get transformed

// Original
import React, {useState} from 'react';

// With this plugin
import React from 'react';
const {useState} = React;

Array destructuring transform for React's built-in hooks

// Original
const [counter, setCounter] = useState(0);

// With this plugin
const __ref__0 = useState(0);
const counter = __ref__0[0];
const setCounter = __ref__0[1];

React.createElement becomes a hoisted constant

// Original
import React from 'react';

function MyComponent() {
  return React.createElement('div', null, 'Hello world');
}

// With this plugin
import React from 'react';
const __reactCreateElement__ = React.createElement;

function MyComponent() {
  return __reactCreateElement__('div', null, 'Hello world');
}

@trueadm trueadm requested a review from gaearon Jan 17, 2019

@apostolos

This comment has been minimized.

Copy link

apostolos commented Jan 17, 2019

It doesn't pick up the correct reference to React when default import is not used (e.g. non-JSX modules).

Let's say you have the following module:

import { useRef, useEffect } from 'react';

export function usePrevious<T>(value: T) {
  const ref: React.MutableRefObject<T | null> = useRef<T>(null);
  useEffect(() => {
    ref.current = value;
  });
  return ref.current;
}

Current output is:

// EXTERNAL MODULE: ./node_modules/react/index.js
var react = __webpack_require__(602);
var react_default = /*#__PURE__*/__webpack_require__.n(react);

// ...

var _React = React,
    useRef = _React.useRef,
    useEffect = _React.useEffect;
function usePrevious(value) {
  var ref = useRef(null);
  useEffect(function () {
    ref.current = value;
  });
  return ref.current;
}

Expected output:

// EXTERNAL MODULE: ./node_modules/react/index.js
var react = __webpack_require__(602);
var react_default = /*#__PURE__*/__webpack_require__.n(react);

// ...

var useRef = react_default.useRef,
    useEffect = react_default.useEffect;
function usePrevious(value) {
  var ref = useRef(null);
  useEffect(function () {
    ref.current = value;
  });
  return ref.current;
}
@trueadm

This comment has been minimized.

Copy link
Author

trueadm commented Jan 17, 2019

@apostolos I've published a new version to NPM – let me know if that fixes your issue.

@apostolos

This comment has been minimized.

Copy link

apostolos commented Jan 17, 2019

@trueadm 0.0.2 fixed the issue with the default import, thanks!

I've found one more issue, although minor. The following is currently broken:

import React from 'react';
import { memo } from 'react';

I get the following error:

ModuleParseError: Module parse failed: Identifier 'React' has already been declared (3:15)
You may need an appropriate loader to handle this file type.
| import React from 'react';
| var __reactCreateElement__ = React.createElement;
> import { memo, React } from 'react';

The question is why bother since you can import both in one statement. It will probably cause issue with TypeScript users that don't use allowSyntheticDefaultImports. The following style is not uncommon:

import * as React from 'react';
import { useRef, useEffect, memo } from 'react';
@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Jan 17, 2019

We definitely want to support all kinds of imports, thanks for reporting.

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Jan 17, 2019

(We should also check that const React = require('react') and similar are at least not broken)

@trueadm

This comment has been minimized.

Copy link
Author

trueadm commented Jan 17, 2019

@apostolos Thanks for reporting, I'll fix that.
@gaearon There are tests for CJS requires. :)

Update: fix published, you can find it on NPM now.

const babel = require('@babel/core');

function transform(code) {
return babel.transform(code, {

This comment has been minimized.

@Andarist

Andarist Jan 17, 2019

this is gonna load babelrc / babel.config.js, not sure if you want that here

also - would be good to have tests both with & without @babel/plugin-transform-destructuring (and possibly some other overlapping combinations)

This comment has been minimized.

@Andarist

Andarist Jan 17, 2019

I see you have no babelrc files in this package, but probably better disable config loading with

babel.transform(code, {
  babelrc: false,
  configFile: false,
  plugins: [plugin],
}).code

just to be more future proof

function createConstantCreateElementReference(reactReferencePath) {
const identifierName = reactReferencePath.node.name;
const binding = reactReferencePath.scope.getBinding(identifierName);
const createElementReference = t.identifier('__reactCreateElement__');

This comment has been minimized.

@Andarist

Andarist Jan 17, 2019

this optimization is OK, but it works on per module (file) basis - it doesnt take into account that production bundles use scope hoisting, maybe we could somehow end up with single constant reference per chunk instead of many?

This comment has been minimized.

@trueadm

trueadm Jan 17, 2019

Author

That would be good, but I was unsure how to do that?

This comment has been minimized.

@Jessidhia

Jessidhia Jan 17, 2019

Don't hardcode the identifier name, use path.scope.generateUidIdentifier in Program and save its value on state (the second argument to a visitor, which also is === this)

This comment has been minimized.

@Jessidhia

Jessidhia Jan 17, 2019

I guess having a hardcoded name actually does let what Andarist suggested work, but not in a way that works with a minifier. You'd have to do var ref = ref || React.createElement over and over and you can't prove to a minifier the result of that expression is guaranteed truthy. Scope hoisting would rename the local vars anyway.

The only surefire way to get this working with scope hoisting is to have an ESM export of React. It is possible to generate one with a webpack plugin, and then use a Babel plugin to rewrite imports of React to import the generated ESM wrapper. (e.g. rewrite "from 'react'" to be "from 'react-esm-loader!react'")

This way, you could ensure that only other ESM would be importing the shared React ESM, and scope hoisting would do its job.

That'd stop working as well if there are dynamic imports, though, as webpack would be forced to put the modules shared with more than one chunk in a separate, non-scope-hoisted module. Perhaps another webpack plugin pass could detect this case and generate one wrapper module per chunk.

This comment has been minimized.

@gaearon

gaearon Jan 18, 2019

Member

We don't enable scope hoisting in CRA. Tbh I think it's reasonable compromise for now.

This comment has been minimized.

@Andarist

Andarist Jan 18, 2019

We don't enable scope hoisting in CRA. Tbh I think it's reasonable compromise for now.

Are you sure? I haven't checked it, but it's enabled by default in webpack@4 in production mode (which is used by CRA). https://webpack.js.org/plugins/module-concatenation-plugin/

That would be good, but I was unsure how to do that?

Me neither 😅 Would have to move this rewrite to other phase than transpilation.

@apostolos

This comment has been minimized.

Copy link

apostolos commented Jan 17, 2019

I think I've found an edge case: This module defines an FC that passes its children prop to createPortal, it does not render JSX directly.

Source: https://github.com/LWJGL/lwjgl3-www/blob/e0ee6d0d47d92c453c1803c6a8b4b0c2ed63a156/client/components/Portal.tsx

The import looked like this:

import { memo, useRef, useEffect } from 'react';

But it breaks like in the first case (_React instead of react_default).

I found the following 3 workarounds:

// 1
import React, { useRef, useEffect } from 'react';
const { memo } = React;
export const Portal = memo(/*...*/);

// 2
import React, { useRef, useEffect } from 'react';
export const Portal = React.memo(/*...*/);

// 3
import React, { memo, useRef, useEffect } from 'react';
export const Portal = memo(/*...render at least some JSX here...*/);

Important detail: If memo is not used at all, it works fine!

@vincentriemer

This comment has been minimized.

Copy link

vincentriemer commented Jan 17, 2019

Gave it a try and it appears not to be working with namespace imports.

I cloned your branch and added this test:

it('should transform React.createElement calls #4', () => {
  const test = `
    import * as React from "react";

    const node = React.createElement("div", null, React.createElement("span", null, "Hello world!"));

    export function MyComponent() {
      return node;
    }
  `;
  const output = transform(test);
  expect(output).toMatchSnapshot();
});

Which results in the following snapshot:

exports[`React createElement transforms should transform React.createElement calls #4 1`] = `
"import * as React, React from \\"react\\";
const node = React.createElement(\\"div\\", null, React.createElement(\\"span\\", null, \\"Hello world!\\"));
export function MyComponent() {
  return node;
}"
`;

Which explains the syntax errors I was getting.

@trueadm

This comment has been minimized.

Copy link
Author

trueadm commented Jan 17, 2019

@apostolos @vincentriemer Thanks for the bug reports. I'll fix them tomorrow. If you want to get involved though – feel free to make a PR against my forked React repro. Any help would be grateful :)

@artemirq

This comment has been minimized.

Copy link

artemirq commented Jan 19, 2019

@trueadm Hey, nice work! What about these Babel plugins? It's relevant now?

@trueadm

This comment has been minimized.

Copy link
Author

trueadm commented Jan 21, 2019

@artemirq Many of those plugins are still relevant but not in the scope right now for this plugin. We may expand this scope in the future.

@trueadm

This comment has been minimized.

Copy link
Author

trueadm commented Jan 21, 2019

@vincentriemer @apostolos I've released a new version of the plugin to NPM. Please let me know if you find anymore issues. Thanks for the great help!

@apostolos

This comment has been minimized.

Copy link

apostolos commented Jan 21, 2019

@trueadm Fixes all remaining issues for me (also tested import * as React from 'react'). Using it in production here: https://www.lwjgl.org/

@trueadm

This comment has been minimized.

Copy link
Author

trueadm commented Jan 21, 2019

@apostolos Awesome stuff. Did you notice and differences compared to before (bundle size, performance)?

@apostolos

This comment has been minimized.

Copy link

apostolos commented Jan 21, 2019

Did a quick comparison on lwjgl.org (latest React alpha, uses only function components with hooks, no classes). I've included react-local which does similar optimizations (although, afaik, without the hook transformation):

All routes with content

Build Minified Minified/GZIP
baseline 1004.45 kB 275.91 kB
react-local 1004.13 kB 276.26 kB
optimize-react 1004.13 kB 276.25 kB

/customize route (more app-like)

Build Minified Minified/GZIP
baseline 66.66 kB 21.39 kB
react-local 66.53 kB 21.55 kB
optimize-react 66.53 kB 21.55 kB

  • Both minify better but compress worse than baseline.
  • I expected more dramatic differences when compiled with spec: true, loose: false, but didn't see any. It might be because Babel supposedly now assumes array when compiling hooks destructuring... I don't know.
  • Didn't see noticeable performance difference between them and profiling didn't help. I suppose it depends on the app and I'm sure we'll see something on an artificial benchmark.

EDIT: Source code available here: https://github.com/LWJGL/lwjgl3-www/tree/master/client

@trueadm

This comment has been minimized.

Copy link
Author

trueadm commented Jan 21, 2019

@apostolos Thanks for checking. :) The differences are all very negligible indeed!

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Jan 21, 2019

To be fair that example uses TS so I'm not sure it's directly comparable to our current setup.

@chrisvasz

This comment has been minimized.

Copy link

chrisvasz commented Jan 22, 2019

This is great, but doesn't appear to play nicely with @babel/preset-env.

babel.config.js:

module.exports = {
  presets: [
    '@babel/react',
    ['@babel/preset-env', { modules: false, targets: 'ie>=11' }],
  ],
  plugins: ['optimize-react'],
};

App.js:

import React, { useState } from 'react';

function App() {
  let [count, setCount] = useState(0);
  return <div onClick={() => setCount(count + 1)}>{count}</div>;
}

output from npx babel App.js (whether or not targets: 'ie>=11' is included):

function _slicedToArray(arr, i) { return _arrayWithHoles(arr) || _iterableToArrayLimit(arr, i) || _nonIterableRest(); }

function _nonIterableRest() { throw new TypeError("Invalid attempt to destructure non-iterable instance"); }

function _iterableToArrayLimit(arr, i) { var _arr = []; var _n = true; var _d = false; var _e = undefined; try { for (var _i = arr[Symbol.iterator](), _s; !(_n = (_s = _i.next()).done); _n = true) { _arr.push(_s.value); if (i && _arr.length === i) break; } } catch (err) { _d = true; _e = err; } finally { try { if (!_n && _i["return"] != null) _i["return"](); } finally { if (_d) throw _e; } } return _arr; }

function _arrayWithHoles(arr) { if (Array.isArray(arr)) return arr; }

import React from 'react';
var __reactCreateElement__ = React.createElement;
var useState = React.useState;

function App() {
  var _useState = useState(0),
      _useState2 = _slicedToArray(_useState, 2),
      count = _useState2[0],
      setCount = _useState2[1];

  return __reactCreateElement__("div", {
    onClick: function onClick() {
      return setCount(count + 1);
    }
  }, count);
}

In this output, it doesn't look like the array destructuring transform happened. If I comment out the line in babel.config.js that includes @babel/preset-env, the output looks much better:

import React from 'react';
const __reactCreateElement__ = React.createElement;
const {
  useState
} = React;

function App() {
  let _ref_0 = useState(0);

  let setCount = _ref_0[1];
  let count = _ref_0[0];
  return __reactCreateElement__("div", {
    onClick: () => setCount(count + 1)
  }, count);
}
@umidbekkarimov

This comment has been minimized.

Copy link

umidbekkarimov commented Feb 6, 2019

Mostly functional components, tons of hooks. Parsed size become lower, gzipped is bigger:

Without `babel-plugin-optimize-react`

Parsed

Show chunks:
All (1.45 MB)
static/js/vendors~main.3aebfb9f.chunk.js (591.12 KB)
static/js/main.f1f1f4a0.chunk.js (416.35 KB)
static/js/vendors~admin.8473e02e.chunk.js (340.38 KB)
static/js/admin.0ebde3f8.chunk.js (135.28 KB)
static/js/bundle.2f4bc937.js (2.24 KB)

Gzipped

All (382.77 KB)
static/js/vendors~main.3aebfb9f.chunk.js (175.99 KB)
static/js/main.f1f1f4a0.chunk.js (106.06 KB)
static/js/vendors~admin.8473e02e.chunk.js (79.7 KB)
static/js/admin.0ebde3f8.chunk.js (19.87 KB)
static/js/bundle.2f4bc937.js (1.15 KB)
With `babel-plugin-optimize-react`

Parsed

Show chunks:
All (1.42 MB)
static/js/vendors~main.3aebfb9f.chunk.js (591.12 KB)
static/js/main.65336000.chunk.js (397.01 KB)
static/js/vendors~admin.8473e02e.chunk.js (340.38 KB)
static/js/admin.3364ead4.chunk.js (126.3 KB)
static/js/bundle.94a853c1.js (2.24 KB)

Gzipped

All (384.5 KB)
static/js/vendors~main.3aebfb9f.chunk.js (175.99 KB)
static/js/main.65336000.chunk.js (107.03 KB)
static/js/vendors~admin.8473e02e.chunk.js (79.7 KB)
static/js/admin.3364ead4.chunk.js (20.63 KB)
static/js/bundle.94a853c1.js (1.15 KB)
@trueadm

This comment has been minimized.

Copy link
Author

trueadm commented Feb 6, 2019

@umidbekkarimov Overall that looks to be a general positive win. Gzip size was marginally down but parsed time is where it really went up. Also Brotli compression should further improve over gzip in the cases where the plugin was enabled.

@ianschmitz ianschmitz referenced this pull request Feb 7, 2019

Open

Support React Hooks #5602

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment