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

Incompatible with React Forget compiler #3874

Open
rikisamurai opened this issue May 16, 2024 · 3 comments
Open

Incompatible with React Forget compiler #3874

rikisamurai opened this issue May 16, 2024 · 3 comments

Comments

@rikisamurai
Copy link

rikisamurai commented May 16, 2024

Intended outcome: success

Actual outcome: Found the following incompatible libraries: mobx
image

How to reproduce the issue:

git clone https://github.com/rikisamurai/compiler-test.git
npm i
npx react-compiler-healthcheck

Versions
6.12.3

@mweststrate
Copy link
Member

mweststrate commented May 16, 2024

At the moment React Forget assumes that all object reads are from immutable objects. E.g. something like <div>{store.todos[0].author.name}</div> is rewritten by Forget to a pointer check on just store before re-rendering the div. So if store is unchanged it assumes statically that todos, author and name also can't have changed. So this interferes with the runtime checking and possibility to mutate objects that MobX offers.

So we're depending on some future option to opt-out form this at object or component level. Until then the architectures are fundamentally incompatible, as far as I can see atm (without severely regressing the DX of MobX).

On the upside, many of the optimisations that are offered now by Forget, were already provided by MobX.

@mweststrate mweststrate changed the title incompatible with react compiler Incompatible with React Forget compiler May 16, 2024
@xaviergonz
Copy link
Contributor

xaviergonz commented May 22, 2024

I've been playing with the compiler playground and the only issues I could find are with a store accessed either from a global directly or through a global getter function (it seems to cache those), but if they come either via props or via a hook (including context) it seems ok, so maybe the solution is to just replace globals with a silly hook?

For example, if we have something like like:
const rootStore = getRootStore()

the user could turn that into

const useRootStore = () => getRootStore()
const rootStore = useRootStore()

and I think he should be ok

https://playground.react.dev/#N4Igzg9grgTgxgUxALhASwLYAcIwC4AEwBUYCAsghhADQlkDKeAhnggQL4EBmMEGBADogYCZnDzDBAOxlwI0sIQD6S3AgBifDAHEANhABGzPQQC8RDgG4Z3KNIloFBAOYI8TdVv76jJgBQAlEQEoniw0gSqeF7avsZ6Vpy29o7OpAgM-JkxokEhYRFRaqLeugYJyQ4KSgSeogDCCmwAHoQWcKKsCE3SrXj+0bE+FSaBMjIILTj4BAAmCNzMUHqEdg54TpHkAJ4AglhY+cAyBATyioQl7BYZWRg56kE2spGh7kUAPPU9zVN4ADoAAp8ABuaAWMAIoJMUAQZmA1w4AD5TmcCJ8mtgCNcyiCIFgwAikQB6VFvM6Y-hYABMONymm0+MJxIZXDJaM+JJ+vX6wLBEIQMGRLw4E1eJJJBAA7mg8AALAjMGAwZg7MApDZbAhYo7EXFMviEzjBE5vC61A38AASEAgAGtzPRMtkfs80RargyyvETE6hqU4qNEh6al7huU-Ho6RY3B5vUGo+7zWH6RHef8nRkM21-Dy-m1Ai8zoUYJFPuT0Ri5mhQcjEQn+MywACYnMIGAANoABgAugDlgrcADpMwHhwuTW62jKVP61aMLaHa2IO2u32B1AhzAR2OEBOSXOZ9Xa-PG5GEiu1z3+4P5cPR+PJ6fj585w2I77o1eOzfN9vdyfQ8XwpE86w-QN+BzQE7wfPcDyPN4uXJMUJSlWUhy3JUVTVDVpHWNJIl1Gl-H1c9mxNIhQ0uNNIMXO1HVuRhXQZZMzk9WjGRGKN-QXL9i3OVM+ODGNXHcH4fWDNjBJohdoKzMhoLzBloKLNFS3LSt0TfU8IK4jBm3-e8d0ffdn2nUCdPAhcl3tIy4KAxCqyss9P2DeyTPg8ytNnXThKjGkPMAszgIs5z3zkgsYK3YzgoQkDKQ5aRUJADggA

Expand the source map section in the right, it is easier to follow that way.

@moritzuehling
Copy link

moritzuehling commented May 22, 2024

This looks indeed very promising! @xaviergonz

As a user of the library:
I agree that some of the optimizations that forget provides are already done by mobx. However, there are quite a few cases we've had in our (rather complex) mobx codebase where I suspect react forget would have helped.

Specifically, it is still very easy to create accidental render cascades with functions (if you don't use useCallback). In addition, I'd classify mobx's strength at avoiding unnecessary render calls with memoization.

Forget is different - it can only run parts of a function, and it can then avoid render cascades to not-impacted parts.

So, overall, we suspect that react-forget would create additional performance improvements for us. That said, I can't measure the impact for various reasons (mainly time constraints).
On the other hand, I do know that I've optimized 20+ hot paths with react-forget style optimizations, and they've helped quite a bit, especially with stuff that needed to run at 60+ FPS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants