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

Moved named hooks code (and tests) from react-devtools-extensions to react-devtools-shared #22260

Merged

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Sep 7, 2021

This is an pre-req for adding the named hooks feature to react-devtools-inline (and eventually the standalone package as well).

This PR is split into three commits to simplify review:

  1. Moved named hooks code (and tests) from react-devtools-extensions to react-devtools-shared
  2. Run yarn update-mock-source-maps in react-devtools-shared
  3. Update packages, imports, and configs for new named hooks location

IMO only the third commit needs to be carefully reviewed.

Why did generated code change?

I'm not sure why the 2nd step generated slight differences in generated code. The changes seem to be two things (both explained below). May be worth reviewers spot checking a few files to make sure nothing else unexpected changed?

The good news is that all tests still pass.

Comment header blocks have been altered

Some are removed entirely:

-/**
- * Copyright (c) Facebook, Inc. and its affiliates.
- *
- * This source code is licensed under the MIT license found in the
- * LICENSE file in the root directory of this source tree.
- *
- * @flow
- */

Others remain but have the @flow pragma comment removed:

/**
 * Copyright (c) Facebook, Inc. and its affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
- * @flow
+ * 
 */

Compiled JSX expression have added __source metadata.

For instance, generated code that previously looked like this:

/*#__PURE__*/React__default.createElement("button", {onClick: () => setCount(count + 1)}, "Click me");

Now looks like this:

/*#__PURE__*/ React__default.createElement(
  "button",
  {
    onClick: () => setCount(count + 1),
    __source: {
      fileName: _jsxFileName$4,
      lineNumber: 18,
      columnNumber: 7,
    },
  },
  "Click me"
);

Brian Vaughn added 3 commits September 7, 2021 11:04
…react-devtools-shared

Note that this is purely a move. It does not update any paths/configs/etc. This is to make it easier to review, but it also means that this commit breaks the code temporarily.
This changes the format of files slightly (e.g. removes Facebook copyirght headers, slightly different Prettier formatting) but all tests pass.
This is the commit that should be reviewed.
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Sep 7, 2021
@sizebot
Copy link

sizebot commented Sep 7, 2021

Comparing: abbc79d...7fa3ab6

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 127.60 kB 127.60 kB = 40.73 kB 40.73 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 130.42 kB 130.42 kB = 41.66 kB 41.66 kB
facebook-www/ReactDOM-prod.classic.js = 405.18 kB 405.18 kB = 75.05 kB 75.05 kB
facebook-www/ReactDOM-prod.modern.js = 393.75 kB 393.75 kB = 73.33 kB 73.33 kB
facebook-www/ReactDOMForked-prod.classic.js = 405.18 kB 405.18 kB = 75.05 kB 75.05 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 7fa3ab6

Copy link
Contributor

@jstejada jstejada left a comment

Choose a reason for hiding this comment

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

thanks! regarding the changes in the generated files, if we run update-mock-source-maps /before/ moving the files around, does that still produce the changes in the files? maybe some dependency got updated that changed the output

@bvaughn
Copy link
Contributor Author

bvaughn commented Sep 7, 2021

thanks! regarding the changes in the generated files, if we run update-mock-source-maps /before/ moving the files around, does that still produce the changes in the files?

Oh, huh. That's a good question!

I'm actually not able to run that command in master because of an error:


/Users/bvaughn/Documents/git/react/packages/react-devtools-shared/src/PerformanceMarks.js:10
import {__PERFORMANCE_PROFILE__} from './constants';
^^^^^^

SyntaxError: Cannot use import statement outside a module
    at wrapSafe (internal/modules/cjs/loader.js:984:16)
    at Module._compile (internal/modules/cjs/loader.js:1032:27)
    at Module._compile (/Users/bvaughn/Documents/git/react/node_modules/pirates/lib/index.js:99:24)
    at Module._extensions..js (internal/modules/cjs/loader.js:1097:10)
    at Object.newLoader [as .js] (/Users/bvaughn/Documents/git/react/node_modules/pirates/lib/index.js:104:7)
    at Module.load (internal/modules/cjs/loader.js:933:32)
    at Function.Module._load (internal/modules/cjs/loader.js:774:14)
    at Module.require (internal/modules/cjs/loader.js:957:19)
    at require (internal/modules/cjs/helpers.js:88:18)
    at Object.<anonymous> (/Users/bvaughn/Documents/git/react/packages/react-devtools-extensions/src/astUtils.js:10:1)
error Command failed with exit code 1.

Not sure what's up here.

maybe some dependency got updated that changed the output

This seems like a reasonable explanation though.

@bvaughn
Copy link
Contributor Author

bvaughn commented Sep 7, 2021

I just deleted packages/react-devtools-shared/src/PerformanceMarks.js and ran the update command in master. No generated files changed. So I'm not sure.

@bvaughn bvaughn merged commit e07039b into facebook:main Sep 7, 2021
@bvaughn bvaughn deleted the react-detools-move-named-hooks-code-to-shared branch September 7, 2021 15:44
bvaughn pushed a commit that referenced this pull request Sep 9, 2021
This commit builds on PR #22260 and makes the following changes:
* Adds a DevTools feature flag for named hooks support. (This allows us to disable it entirely for a build via feature flag.)
* Adds a new Suspense cache for dynamically imported modules. (This allows a component to suspend while importing an external code chunk– like the hook names parsing code).
* DevTools supports a hookNamesModuleLoaderFunction param to import the hook names module. I wish this could be handles as part of the react-devtools-shared package, but I'm not sure how to configure Webpack (4) to serve the chunk from react-devtools-inline. This seemed like a reasonable workaround.

The PR also contains an additional unrelated change:
* Removes pre-fetch optimization (added in DevTools: Improve named hooks network caching #22198). This optimization was mostly only important for cases where sources needed to be re-downloaded, something which we can now avoid in most cases¹ thanks to using cached responses already loaded by the page. (I tested this locally on Facebook and this change has no negative performance impact. There is still some overhead from serializing the JS through the Bridge but that's constant between the two approaches.)

¹ The case where we don't benefit from cached responses is when DevTools are opened after the page has already loaded certain scripts. This seems uncommon enough that I don't think it justified the added complexity of prefetching.
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
This commit builds on PR facebook#22260 and makes the following changes:
* Adds a DevTools feature flag for named hooks support. (This allows us to disable it entirely for a build via feature flag.)
* Adds a new Suspense cache for dynamically imported modules. (This allows a component to suspend while importing an external code chunk– like the hook names parsing code).
* DevTools supports a hookNamesModuleLoaderFunction param to import the hook names module. I wish this could be handles as part of the react-devtools-shared package, but I'm not sure how to configure Webpack (4) to serve the chunk from react-devtools-inline. This seemed like a reasonable workaround.

The PR also contains an additional unrelated change:
* Removes pre-fetch optimization (added in DevTools: Improve named hooks network caching facebook#22198). This optimization was mostly only important for cases where sources needed to be re-downloaded, something which we can now avoid in most cases¹ thanks to using cached responses already loaded by the page. (I tested this locally on Facebook and this change has no negative performance impact. There is still some overhead from serializing the JS through the Bridge but that's constant between the two approaches.)

¹ The case where we don't benefit from cached responses is when DevTools are opened after the page has already loaded certain scripts. This seems uncommon enough that I don't think it justified the added complexity of prefetching.
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.

4 participants