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 React JSX runtime to default exceptions for inline requires #1126

Closed
wants to merge 1 commit into from

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Oct 30, 2023

Summary

JSX runtime calls are all over the place in every components. When using inline requires, it doesn't make sense to go through the method call + array access + property access just to call the JSX factory.

Test plan

Before (with inline requires on):

  Object.defineProperty(exports, '__esModule', {
    value: true,
  });
  var _reactNative = _$$_REQUIRE(_dependencyMap[0], "react-native"),
    View = _reactNative.View,
    Text = _reactNative.Text;
  function App() {
    return /*#__PURE__*/_$$_REQUIRE(_dependencyMap[1], "react/jsx-runtime").jsx(View, {
      style: {
        flex: 1,
        backgroundColor: 'green'
      },
      children: /*#__PURE__*/_$$_REQUIRE(_dependencyMap[1], "react/jsx-runtime").jsxs(Text, {
        style: {
          flex: 1
        },
        children: ["whatever ", _$$_IMPORT_ALL(_dependencyMap[2], "./test").A]
      })
    });
  }

After (with inline requires on):

  Object.defineProperty(exports, '__esModule', {
    value: true,
    lol: true
  });
  var _reactNative = _$$_REQUIRE(_dependencyMap[0], "react-native"),
    View = _reactNative.View,
    Text = _reactNative.Text;
  var _jsxs = _$$_REQUIRE(_dependencyMap[1], "react/jsx-runtime").jsxs;
  var _jsx = _$$_REQUIRE(_dependencyMap[1], "react/jsx-runtime").jsx;
  function App() {
    return /*#__PURE__*/_jsx(View, {
      style: {
        flex: 1,
        backgroundColor: 'green'
      },
      children: /*#__PURE__*/_jsxs(Text, {
        style: {
          flex: 1
        },
        children: ["whatever ", _$$_IMPORT_ALL(_dependencyMap[2], "./test").A]
      })
    });
  }

I added both react/jsx-runtime and react/jsx-dev-runtime even though currently RN only uses the former.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 30, 2023
@facebook-github-bot
Copy link
Contributor

@robhogan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@gaearon
Copy link
Contributor Author

gaearon commented Oct 30, 2023

We also ideally need to do this for @babel/runtime helpers. But there are a lot of those. Ideally it would be nice to be able to blocklist an entire package from inline requires. How do we feel about that?

@gaearon
Copy link
Contributor Author

gaearon commented Oct 31, 2023

Chose a different approach for the other change: #1127

@facebook-github-bot
Copy link
Contributor

@robhogan merged this pull request in c463b64.

facebook-github-bot pushed a commit that referenced this pull request Oct 31, 2023
Summary:
There is no benefit to treating Babel helpers with lazy requires. They are always going to be needed, and their initialization is extremely cheap, so it just bloats up the code and adds extra runtime indirection on every access to them.

I considered a few alternatives:

- We could add them to a default list like #1126. But there's far too many to list individual modules.
- We could add wildcard support to `nonInlinedRequires` option.
- We could add regex support to `nonInlinedRequires` option.
- We could add a new option.

I'm not particularly enthusiastic about doing the bigger changes myself. I think hardcoding it here is fine because there's basically no legit reason for wanting these to be lazy. In fact they might even not be lazy anyway depending on where in your compilation pipeline the runtime plugin is injected. But this is nice because it fixes it for all RN users with inline requires.

Pull Request resolved: #1127

Test Plan:
Ran my app with the change. The app works. The diff looks like I'd expect, for example:

<img width="1144" alt="Screenshot 2023-10-31 at 00 40 00" src="https://github.com/facebook/metro/assets/810438/c04f4f5c-0f41-489b-aa50-fac9286f4c1d">

Reviewed By: motiz88

Differential Revision: D50828195

Pulled By: robhogan

fbshipit-source-id: b05d26dd26431863deadfae7631913d4dd747a05
@gaearon gaearon deleted the patch-1 branch November 3, 2023 23:59
facebook-github-bot pushed a commit that referenced this pull request Nov 18, 2024
Summary:
Similar to #1126. This call is in every component on every render and it is not beneficial to inline it. On the contrary, you want to reduce the indirection there.

Changelog: [Performance] Exclude React Compiler runtime from inline requires

<!--
Changelog entries should be prefixed with one of the following scopes:
[Breaking, Feature, Fix, Performance, Deprecated, Experimental, Internal]

Examples:
  Changelog: [Fix] Respond with HTTP 404 when a bundle entry point cannot be resolved
  Changelog: [Internal]
-->
Changelog:

Pull Request resolved: #1390

Test Plan:
Before:

<img width="846" alt="Screenshot 2024-11-17 at 18 01 25" src="https://github.com/user-attachments/assets/3e04a6b0-dd41-4374-8872-bfc5a1982866">

After:

<img width="814" alt="Screenshot 2024-11-17 at 18 08 05" src="https://github.com/user-attachments/assets/fb9cd5dc-2f50-440a-a175-1859e63752a9">

Reviewed By: GijsWeterings

Differential Revision: D66075145

Pulled By: robhogan

fbshipit-source-id: 11a12fbd04620bdb3e5b9320aca8a02c1f05803f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants