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

[Fresh] Fix edge case with early function call #17824

Merged
merged 1 commit into from
Jan 12, 2020

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Jan 12, 2020

When a component calls custom Hooks, we "collect" a signature in two steps.

// This is a wrapper over more primitive functions for setting signature.
// Signatures let us decide whether the Hook order has changed on refresh.
//
// This function is intended to be used as a transform target, e.g.:
// var _s = createSignatureFunctionForTransform()
//
// function Hello() {
// const [foo, setFoo] = useState(0);
// const value = useCustomHook();
// _s(); /* Second call triggers collecting the custom Hook list.
// * This doesn't happen during the module evaluation because we
// * don't want to change the module order with inline requires.
// * Next calls are noops. */
// return <h1>Hi</h1>;
// }
//
// /* First call specifies the signature: */
// _s(
// Hello,
// 'useState{[foo, setFoo]}(0)',
// () => [useCustomHook], /* Lazy to avoid triggering inline requires */
// );

For example, given this component:

function App() {
  const x = useFancyState('x')
  const y = useFancyState('x')
}

function useFancyState(value) {
  return React.useState(value)
}

we want to:

  1. First, establish that App has two useFancyState calls.
    • Our transform does this with a call like _s(App, 'useFancyState(x) useFancyState(y)', () => [useFancyState, useFancyState])
  2. Then, when we're inside App for the first time, trigger the function to save their identities
    • Our transform does this with a call like _s()

We intentionally trigger the function to save function identities lazily on first render. This is so that we don't change the module definition order when you have inline requires.

We reuse the same function to minimize the transform noise. So we count calls. First call saves () => [useFancyState, useFancyState], second call actually triggers it to get references. Next calls don't do anything.

This usually works, but it breaks in the corner case like this:

render(<App />);

function App() { ... }
function useFancyState() { ... }

This is because we end up inside App before the first call injected by our transform runs. So we get the "inside" call before we get the "outside" call.

This is edge casey but can happen in environments like CodeSandbox where you write all code in one file.

The fix is to not rely on the call index, and instead to look at what we're passing. With this fix, we ignore all empty _s() calls until after we actually get the first _s(App, ...) registration call. To make this clearer, I'm changing a number tracking the call number to a status enum.

A regression test is added. This should fix this broken case on CodeSandbox. (After CodeSandbox updates react-refresh to the version with this fix.)

@gaearon gaearon requested a review from bvaughn January 12, 2020 15:44
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jan 12, 2020
@codesandbox-ci
Copy link

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 e47cd31:

Sandbox Source
misty-violet-i7gmr Configuration

@sizebot
Copy link

sizebot commented Jan 12, 2020

Details of bundled changes.

Comparing: 64aae7b...e47cd31

react-refresh

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-refresh-runtime.development.js +3.9% +3.9% 18.41 KB 19.13 KB 5.49 KB 5.71 KB NODE_DEV
react-refresh-runtime.production.min.js 0.0% 🔺+0.4% 368 B 368 B 264 B 265 B NODE_PROD
react-refresh-babel.development.js 0.0% 0.0% 24.06 KB 24.06 KB 5.49 KB 5.5 KB NODE_DEV
react-refresh-babel.production.min.js 0.0% 0.0% 7.19 KB 7.19 KB 2.57 KB 2.57 KB NODE_PROD

Size changes (stable)

Generated by 🚫 dangerJS against e47cd31

@sizebot
Copy link

sizebot commented Jan 12, 2020

Details of bundled changes.

Comparing: 64aae7b...e47cd31

react-refresh

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-refresh-babel.development.js 0.0% 0.0% 24.07 KB 24.07 KB 5.5 KB 5.5 KB NODE_DEV
react-refresh-babel.production.min.js 0.0% 0.0% 7.2 KB 7.2 KB 2.58 KB 2.58 KB NODE_PROD
ReactFreshRuntime-dev.js +3.9% +4.0% 18.49 KB 19.21 KB 5.48 KB 5.7 KB FB_WWW_DEV
react-refresh-runtime.development.js +3.9% +3.9% 18.42 KB 19.14 KB 5.5 KB 5.72 KB NODE_DEV

Size changes (experimental)

Generated by 🚫 dangerJS against e47cd31

@@ -601,9 +601,14 @@ export function _getMountedRootCount() {
// 'useState{[foo, setFoo]}(0)',
// () => [useCustomHook], /* Lazy to avoid triggering inline requires */
// );
type SignatureStatus = 'needsSignature' | 'needsCustomHooks' | 'resolved';
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice readability change 👍

@gaearon gaearon merged commit 255d9ac into facebook:master Jan 12, 2020
@gaearon gaearon deleted the fresh-fix branch January 12, 2020 17:54
@thgh
Copy link

thgh commented May 22, 2020

Hi, I'm having this issue in Expo which seems to be using version 0.4.2. I'm using patch-package for now to add this line:

diff --git a/node_modules/react-refresh/cjs/react-refresh-runtime.development.js b/node_modules/react-refresh/cjs/react-refresh-runtime.development.js
index 3e017a2..27b49af 100644
--- a/node_modules/react-refresh/cjs/react-refresh-runtime.development.js
+++ b/node_modules/react-refresh/cjs/react-refresh-runtime.development.js
@@ -279,6 +279,7 @@ function register(type, id) {
   }
 }
 function setSignature(type, key) {
+  if (!key) return
   var forceReset = arguments.length > 2 && arguments[2] !== undefined ? arguments[2] : false;
   var getCustomHooks = arguments.length > 3 ? arguments[3] : undefined;

Would you mind to release this fix as 0.4.3?

Based on my yarn.lock file, the other dependencies always use react-refresh@^0.4.0 and I tried to upgrade to 0.7.2 (where the fix was introduced) and 0.8, but those versions are not compatible.

@gaearon
Copy link
Collaborator Author

gaearon commented May 22, 2020

What is the actual code triggering this in your case?

@gaearon
Copy link
Collaborator Author

gaearon commented May 22, 2020

I released this PR as 0.4.3.

@thgh
Copy link

thgh commented May 23, 2020

Awesome! It works 😃 actual code:

const App = () => <Text>{useUser()}</Text>

// imported
const [useUser, setUser] = createGlobalHook<string>('')
export { useUser, setUser }
fetchUser().then(setUser)

// imported
export function createGlobalHook(state) {
  let effects: any[] = []
  return [useGlobal, setGlobal, get]

  function useGlobal() {
    const [, effect] = useState(state)
    useEffect(() => {
      effects = effects.concat(effect)
      return () => effects = effects.filter(eff => eff !== effect)
    }, [])
    return state
  }

  function setGlobal(newState) {
    if (typeof newState === 'function') {
      state = newState(state)
    } else {
      state = newState
    }
    effects.forEach(s => s(state))
    return state
  }

  function get() {
    return state
  }
}

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.

5 participants