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] Generate signatures for Hooks #15733

Merged
merged 10 commits into from
May 29, 2019
Merged

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented May 24, 2019

This adds support for editing components with Hooks.

State preservation already works by default. The goal here is to not preserve state in cases where it is known to lead to problems. Such as reordering or deleting Hooks. We want to detect when that happens and remount.

This is implemented by generating __signature__ calls for functions that contains calls to Hooks:

function ButtonV1() {
  const [pressed, setPressed] = useState(false)
}
__signature__(ButtonV1, 'useState{[pressed, setPressed]}')

// -----------
// After edit
// -----------

function ButtonV1() {
  const [hover, setHover] = useState(false)
  const [pressed, setPressed] = useState(false)
  // ...
}
__signature__(ButtonV1, 'useState{[hover, setHover]}\nuseState{[pressed, setPressed]}')

If a signature changes, the Fresh runtime will tell React to remount that component type.

The signature contains names of all Hooks. Hooks that have a left hand side like const [foo, setFoo] have its source embedded in the signature. This catches renames (which often indicate invasive edits like changing state variable type).

If you use custom Hooks, we add them as the last argument:

import useMyStuff from './useMyStuff'

function Foo() {
  const stuff = useMyStuff()
}
__signature__(
  Foo,
  'useMyStuff{stuff}',
  () => [useMyStuff] // <--- references, not strings!
);

This lets us recurse into each custom Hook when comparing signatures, so that if useMyStuff itself gets edited, we can remount the components using it.

The last argument is lazily evaluated (a function) for two reasons:

  1. The reference might not be valid at the definition time because the Hook itself could be declared later in the file.
  2. In React Native, we'll want to avoid triggering inline requires at module init time to avoid differences with production behavior.

The PR is easier to read without whitespace because some logic moved. You might also want to check commits separately.

This currently only works one level deep. For custom Hooks, we'll need to add some way to compose signatures.
@sizebot
Copy link

sizebot commented May 24, 2019

Warnings
⚠️

Could not find build artifacts for base commit: 3939248

Generated by 🚫 dangerJS

case 'useCallback':
case 'React.useCallback':
case 'useImperativeMethods':
case 'React.useImperativeMethods':
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the decision to omit useRef, useContext, and useDebugValue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I forgot about them lol

Copy link
Contributor

Choose a reason for hiding this comment

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

😂

// let Foo = () => {}
// let Foo = function() {}
// We'll register it on next line so that
// we don't mess up the inferred 'Foo' function name.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.

).toMatchSnapshot();
});

it('includes custom hooks into the signatures', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

@gaearon gaearon merged commit 556cc6f into facebook:master May 29, 2019
@@ -1,5 +1,61 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`ReactFreshBabelPlugin generates signatures for function declarations calling hooks 1`] = `
"
Copy link
Contributor

@SimenB SimenB Jun 3, 2019

Choose a reason for hiding this comment

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

you can use https://www.npmjs.com/package/jest-snapshot-serializer-raw to avoid the escaped " and extra wrapping ". We've done the same in Jest: jestjs/jest#7651

Makes them a bit easier to read, IMO 🙂

EDIT: Simple enough to PR, so I did: #15806

gaearon added a commit to gaearon/react that referenced this pull request Jun 11, 2019
* Generate signatures for Hooks

This currently only works one level deep. For custom Hooks, we'll need to add some way to compose signatures.

* Be more resilient to plugin conflicts

This prevents a class of problems where other plugins cause our visitor to re-run.

It's a standard Babel practice, e.g.:

https://github.com/babel/babel/blob/8c7d4b55c99ff34cb9d493d452472e59b5ed1e70/packages/babel-plugin-transform-react-constant-elements/src/index.js#L85-L86

* Remove unnecessary stuff from debugging

* Include Foo.useHookName() calls into the signature

* Add an integration test for adding/removing an effect

* Add integration test for changing custom Hook order

* Include custom Hooks into the signatures

* Fix inferred names for function expressions

* Support export default hoc(Foo) when Foo is defined separately

* Add more built-in Hooks
gaearon added a commit to gaearon/react that referenced this pull request Jun 19, 2019
* Generate signatures for Hooks

This currently only works one level deep. For custom Hooks, we'll need to add some way to compose signatures.

* Be more resilient to plugin conflicts

This prevents a class of problems where other plugins cause our visitor to re-run.

It's a standard Babel practice, e.g.:

https://github.com/babel/babel/blob/8c7d4b55c99ff34cb9d493d452472e59b5ed1e70/packages/babel-plugin-transform-react-constant-elements/src/index.js#L85-L86

* Remove unnecessary stuff from debugging

* Include Foo.useHookName() calls into the signature

* Add an integration test for adding/removing an effect

* Add integration test for changing custom Hook order

* Include custom Hooks into the signatures

* Fix inferred names for function expressions

* Support export default hoc(Foo) when Foo is defined separately

* Add more built-in Hooks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants