-
Notifications
You must be signed in to change notification settings - Fork 9
feat: add react-compiler #141
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
25e184c
to
3faa93c
Compare
3faa93c
to
c78ca88
Compare
c78ca88
to
d68dec7
Compare
d68dec7
to
4e11fb2
Compare
4e11fb2
to
4bb8fc9
Compare
src/useObservable.ts
Outdated
() => { | ||
const instance = cache.get(observable)! | ||
if (instance.error) { | ||
throw instance.error | ||
} | ||
return instance.snapshot | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the subscribe
argument in useSyncExternalStore
needs to be memoized, as it'll run teardown + setup every time it changes. The getSnapshot
and getServerSnapshot
arguments don't have to be memoized and the React Compiler uses less memory if we only memoize subscribe
🙌
4bb8fc9
to
26341f9
Compare
26341f9
to
9483f6e
Compare
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "react-rx", | |||
"version": "4.0.1", | |||
"version": "4.1.0-canary.5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ignored by semantic-release
@@ -75,6 +75,7 @@ | |||
"prettier": "@sanity/prettier-config", | |||
"dependencies": { | |||
"observable-callback": "^1.0.3", | |||
"react-compiler-runtime": "19.0.0-beta-6fc168f-20241025", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The guidance atm is to use exact versions, generated like:
pnpm add react-compiler-runtime@beta --save-exact
@@ -106,7 +109,7 @@ | |||
"vitest": "^2.1.4" | |||
}, | |||
"peerDependencies": { | |||
"react": "^18.3 || >=19.0.0-rc", | |||
"react": "^18.3 || >=19.0.0-0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
19.0.0-0
is more permissive than 19.0.0-rc
, and for example permits the canary
versions of 19 that are used on canary builds of next
.
}, | ||
} | ||
}, [observable]) | ||
const subscribe = useCallback( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rewrite to useCallback
on just subscribe
, instead of keeping the useMemo
, is to avoid agressive subscribe + unsubscribe loops.
The Compiler looks at what is actually used inside the useMemo
, which is instance.observable
, instance.snapshot
and instance.error
.
In a sense, it's as if it rewrote our code to be:
const instance = useMemo(() => cache.get(observable), [observable])
const store = useMemo(() => {
// ...
}, [instance.observable, instance.snapshot, instance.error])
This is causing store
to change identity every time snapshot
changes.
So by splitting them up, to use useCallback
instead, this is what happens instead (conceptually):
const instance = useMemo(() => cache.get(observable), [observable])
const subscribe = useCallback(() => {
// ...
}, [instance.observable])
And now we avoid the problem, as instance.observable
is stable. useSyncExternalStore
doesn't care if getSnapshot
or getServerSnapshot
changes identity between renders, it only requires subscribe
to be memoized correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this is great - thanks for the explainer!
plugins: [ | ||
react({ | ||
babel: {plugins: [['babel-plugin-react-compiler', {target: '18'}]]}, | ||
}), | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's vital that the test suite is also using the react compiler, to ensure we're testing the behaviour of the code we're shipping, not the unoptimised code.
Especially since our testing suite is testing implementation details around memoization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing stuff! LGTM ✨
Now that the React Compiler is in Beta, and fully supports targeting React 18 (and above), we can start leveraging it to boost the performance, and reduce the memory footpring, of our libraries 🎉
I've tested this build
[email protected]
on the Studio and tests are passing 🥳It doesn't seem like the eFPS testing suite is setup to capture the performance benefits atm, compare a regular bump with this one and the differences are so small I think they're inconclusive.
It's really cool to look at how the compiler is optimizing the code, how granular it is.
For example it even hoists out inline functions when it can, like this:
And finally, note how it removes the
useMemo
entirely, and replaces it with granular, low level API:Which knows ahead of time exactly how much cache it'll need:
And here's how that's used to ensure that
cache.get(observable)
is only called whenobservable
has changed: