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

Add Auto Import to Babel Plugin #16626

Merged
merged 43 commits into from
Feb 13, 2020
Merged

Conversation

lunaruan
Copy link
Contributor

@lunaruan lunaruan commented Aug 30, 2019

This babel transform is a fork of the @babel/plugin-transform-react-jsx transform and is for experimentation purposes only. We don't plan to own this code in the future, and we will upstream this to Babel at some point once we've proven out the concept.

As per the RFC to simplify element creation, we want to add the ability to auto import "react' directly from the babel plugin. This commit updates the babel plugin with two options:

1.) importSource: The React module to import from. Defaults to react.
2.) shouldCacheImportFns: If this is set to true, React functions that are imported will be cached at the top of the file (ex. var _jsx = _react.jsx) instead of calling the member expression (ex. _react.jsx) directly in the code. Defaults to false.
3.) autoImport: The type of import. Defaults to none.
- none: Does not import React. JSX compiles to React.jsx etc.
- namespace: import * as _react from "react";. JSX compiles to _react.jsx etc.
- default: import _default from "react"; JSX compiles to _default.jsx etc.
- namedExports: import {jsx as _jsx} from "react"; JSX compiles to _jsx etc.
- require: var _react = _interopRequireWildcard(require("react"));. jSX compiles to _react.jsx etc.

namespace, default, and namedExports can only be used when sourceType: module and require can only be used when sourceType: script.

It also adds two pragmas (jsxAutoImport and jsxImportSource) that allow users to specify autoImport and importSource in the docblock.

We plan to test #16432 before landing this diff.

@sebmarkbage
Copy link
Collaborator

@Jessidhia Why can’t we just emit a require call without the interop since we’re a CommonJS module in this mode?

@Jessidhia
Copy link
Contributor

Jessidhia commented Sep 4, 2019

It's possible by giving options to the helper-module-imports calls (3rd argument).

https://github.com/babel/babel/blob/v7.5.5/packages/babel-helper-module-imports/src/import-injector.js#L21-L62

I'm not 100% sure this is public API as it's only documented here, but it could as well just be an omission from the docs since the jsdoc is pretty comprehensive.

exports[`transform react to jsx auto import namespace 1`] = `
import * as _react from "react";

var x = _react.jsx(_react.Fragment, {
Copy link
Collaborator

@sebmarkbage sebmarkbage Sep 4, 2019

Choose a reason for hiding this comment

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

We could add an additional option to emit a variable to cache these at the top of the file.

import * as _react from "react";
const _jsx = _react.jsx;
const _fragment = _react.Fragment;

...

var x = _jsx(_Fragment, {

For a native ES module system this wouldn't matter but the way most module transpilers are implemented this becomes an object. Which may need an indirection to load the object and then the current function in the object for each call. Instead of just loading the current function.

Note that this also applies to namedExports. It should be something like:

import {jsx as __jsx, Fragment as __Fragment} from "react";
const _jsx = __jsx;
const _fragment = __Fragment;

Modern browser VMs may optimize for the inline cache containing this information perhaps. So I don't think this matters but it might for Hermes.

Additionally this approach would slow down initialization of the module scope and use more memory.

So I doubt this approach will actually be better overall but it would be nice to have an option so we can experiment with it and test which one is faster in practice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto for shadowing question and tests on these variables if we add this!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe I tested this once with React.createElement in V8 and found it didn't make a significant difference (IIRC slightly worse; definitely wasn't better or I would've gone through with it).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yea I tested it as well and came to the same conclusion. So we’re not doing this.

The key is that as long as the VM optimizes for storing methods on the object hidden class it’s guaranteed to be worse once you use more than one method.

What I’m wondering next is if it would be worth while having React be a global property.

state.source,
);

const newName = path.scope.generateUidIdentifier(importName);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know too much about generateUidIdentifier but it seems like it's limited to the scope you call it in. How much do you care about shadowing? I can imagine a number of ways that _react could be created in the file but in different scopes that might mess with your usage here. Does generateUidIdentifier handle all possible scopes in the file? Would be good to have a test if it does! Here's a tricky-ish example:

const Component = ({thing, ..._react}) => {
  if (!thing) {
    var _react2 = "something useless";
    return <div />;
  };
  return <span />;
};

Copy link
Contributor Author

@lunaruan lunaruan Sep 5, 2019

Choose a reason for hiding this comment

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

generateUidIdentifier would generate an identifier that doesn't collide with any locally defined variables, so it doesn't just consider the scope it was called in. So in that case it generates:

import * as _react3 from "react";
var _jsx = _react3.jsx;

const Component = function ({
  thing,
  ..._react
}) {
  if (!thing) {
    var _react2 = "something useless";
    return _jsx("div", {});
  }

  ;
  return _jsx("span", {});
};

I added a couple of tests to verify this!

exports[`transform react to jsx auto import namespace 1`] = `
import * as _react from "react";

var x = _react.jsx(_react.Fragment, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto for shadowing question and tests on these variables if we add this!

@necolas necolas added the React Core Team Opened by a member of the React Core Team label Jan 8, 2020
@lunaruan
Copy link
Contributor Author

This PR has not been abandoned. Still working on adding it to WWW.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 12, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ac7d715:

Sandbox Source
vigorous-yalow-sp1ul Configuration

@sizebot
Copy link

sizebot commented Feb 12, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against ac7d715

@sizebot
Copy link

sizebot commented Feb 12, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against ac7d715

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

Should probably fix lints and land this.

@facebook facebook deleted a comment from lemodadada Feb 12, 2020
@lunaruan lunaruan merged commit d4f2b03 into facebook:master Feb 13, 2020
@lunaruan lunaruan deleted the add_auto_import branch February 13, 2020 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants