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] Hash signatures #16738

Merged
merged 1 commit into from
Sep 10, 2019
Merged

[Fresh] Hash signatures #16738

merged 1 commit into from
Sep 10, 2019

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Sep 10, 2019

Fresh Babel plugin emits "signatures" that are strings containing names of Hooks and call metadata. For example:

export default function App() {
  _s();

  const [foo, setFoo] = useState(0);
  React.useEffect(() => {});
  return <h1>{foo}</h1>;
}

_s(App, "useState{[foo, setFoo](0)}\\nuseEffect{}"); // <-- signature

Note the signature contains [foo, setFoo](0). This ensures that if we change the foo name, or if we change the initial state (0), we force a remount. Including the initial argument source code is a special case for useState and useReducer since that usually signals an intentional attempt to remount.

Unfortunately, it's not always safe to emit an arbitrary string in some environments. For example, in www we have a transform that replaces cx("foo") with some string hash regardless of whether it's inside a string or not. It's just a regex. So if an expression like this becomes a part of a signature, www transforms can produce a syntax error.

To fix this, we can default to always emitting hashes for signatures. They're guaranteed to be short, and can't cause these kinds of issues. I left a fallback for ASTExplorer and for our own snapshot tests. (However, our integration tests verify the other branch still works.)

@sizebot
Copy link

sizebot commented Sep 10, 2019

Details of bundled changes.

Comparing: fd3e8cb...601a9ae

react-refresh

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-refresh-runtime.development.js 0.0% +0.1% 16.53 KB 16.53 KB 5.08 KB 5.08 KB NODE_DEV
react-refresh-runtime.production.min.js 0.0% 🔺+0.4% 368 B 368 B 263 B 264 B NODE_PROD
react-refresh-babel.development.js +2.5% +4.8% 23.4 KB 23.98 KB 5.22 KB 5.47 KB NODE_DEV
react-refresh-babel.production.min.js 🔺+2.6% 🔺+4.1% 7 KB 7.19 KB 2.46 KB 2.56 KB NODE_PROD

Generated by 🚫 dangerJS against 601a9ae

Copy link
Contributor

@threepointone threepointone left a comment

Choose a reason for hiding this comment

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

#m nakazawa_yeah

@@ -300,7 +300,20 @@ export default function(babel, opts) {
}
});

const args = [node, t.stringLiteral(key)];
let finalKey = key;
if (typeof require === 'function' && !opts.emitFullSignatures) {
Copy link
Contributor

Choose a reason for hiding this comment

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

(Only because I've been looking for examples for the elvis operator, feel free to ignore)

Suggested change
if (typeof require === 'function' && !opts.emitFullSignatures) {
if (typeof require === 'function' && !opts?.emitFullSignatures) {

Then you wouldn't need the default arguments for opts

Copy link
Collaborator Author

@gaearon gaearon Sep 10, 2019

Choose a reason for hiding this comment

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

Wanna make sure I won't mess it up in other places where it's being read

@gaearon gaearon merged commit ba6bb0f into facebook:master Sep 10, 2019
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.

4 participants