fix error with hot module replacement

[PatrykWalach]

When hot module replacement happens the parentCache doesn't change but useEffect is rerun (don't know what the exact cause of this is), when useEffect is rerun it crashes, because it can't access cacheItem anymore.

The extra check makes sure effect is run once per parentCache (maybe we are compatible with StrictMode now?)

[rbalicki2]

https://isograph.dev/docs/faq/#why-does-isograph-not-support-strict-mode

[PatrykWalach]

Now useLazyDisposableState should work correctly, it will setup cleanup once for every parentCache

[PatrykWalach]

This is not erroring, but the environment cache will indeed produce stale components

PS. so if we fix component cache we will receive fresh components, and it shouldn't cause these errors

PPS. the issue seems to be components get hot reloaded, but somehow iso("entrypoint ...") does not

[rbalicki2]

It's interesting that iso(entrypoint) calls don't get HMR'd, because (I think) that works with Relay, and we don't do much that's different`

[PatrykWalach]

If you place console.log after a resolver it will retrigger on update but if you put it in any artifact importing the resolver it will not retrigger.

[rbalicki2]

weird

[PatrykWalach]

This post from Dan Abramov explains pretty much exactly how HMR works. The identity of function rendered <Component /> matters, the way we wrap components in getOrCreateCachedComponent we lose their identity. For it to work we would need to render resolvers:

- readerWithRefetchQueries.readerArtifact.resolver(firstParam, additionalRuntimeProps)
+ <readerWithRefetchQueries.readerArtifact.resolver {...firstParameter} />

We can't do that because we will lose the second parameter.

Solutions:

1. We could change iso to have runtime impact and wrap the resolver

   let firstParam = Symbol() // maybe symbol works with react?
   const iso = Component => (props) => {
     return Component(props[firstParam], props)
   }

   then HMR registers wrapped component

   const Pokemon = iso(({ data, props }) => /* ... */)
   
   __register__(Pokemon, "Poekemon")

   and we can render it as mentioned above.
2. We can hook into the react-refresh and rely on its implementation details or create our version.

[rbalicki2]

Great find. The gist describes several possible approaches. Do you know which one is actually used by HMR? Is the HMR code adding __register__(PetUpdater, PetUpdater.ts$PetUpdater)?

• Will the HMR code understand to add that, even though our components don't look like components? They're wrapped in iso, for example.
• I don't understand what's going on. But it makes sense that the caching in the component cache is problematic.

What I see

On localhost:3000 and on main:

• If I modify HomeRoute, the specific error that I see is that the cache item is accessed before initialization.
• If I modify PetSummaryCard, I see that HMR occurs, but it doesn't reflect any changes.
• This is different than the error you see, which I saw in the past

Random thoughts about two params

• My instinct is to keep the two parameters, because
  • That's what makes it easiest to provide types to the second parameter
  • It means data, etc. aren't special props within the same namespace
  • We preserve our ability to add future props, like parameters, refetch etc
• But I'm certainly open to the consider making these regular ol' components
• It might be worth experimenting with clobbering everything into a single parameter and seeing if that fixes the HMR issues

Our own integration with HMR

• It 100% makes sense that the cache in componentCache is the problem. That's global state that's not reset when we do HMR.
• I was thinking that we should move that caching into the Isograph store, so that we get garbage collection and other such benefits for free. But we should consider whether that is compatible with HMR.
• Given all of that, maybe it makes sense for our "HMR integration" to be just deleting items in the Isograph store/out of the component cache? Just a thought. I have no idea if that will work

[rbalicki2]

We see that ReferenceError: Cannot access 'cacheItem' before initialization, which means that the effect is called but we returned early (ie. (lastCommittedParentCache.current === parentCache)).

Okay, just noticing that. I don't know what that implies

[PatrykWalach]

It 100% makes sense that the cache in componentCache is the problem. That's global state that's not reset when we do HMR.

Keeping stale references to resolvers is not an issue.

I don't understand what's going on. But it makes sense that the caching in the component cache is problematic.

The way HMR works is every component refers to stale <SubComponent /> but react under the hood has a Map<StaleComponent, LatestComponent>, whenever a file is modified it updates the entry in the map with __register__ and rerenders with react.

By calling as a function StaleComponent() instead of rendering <StaleComponent /> we skip the step of react reading from the Map

[PatrykWalach]

We see that ReferenceError: Cannot access 'cacheItem' before initialization, which means that the effect is called but we returned early (ie. (lastCommittedParentCache.current === parentCache)).

This means you probably didn't run pnpm -r compile because this is an error that should be fixed here.

[rbalicki2]

Oh sorry, I was on main. Maybe that's why we were seeing different errors

[rbalicki2]

By calling as a function StaleComponent() instead of rendering we skip the step of react reading from the Map

Ohhh sorry. I hadn't read the latest changes in your PR. That's a helpful description.

So HMR is "working", and that's re-running the effect, and the effect is being run despite the component returning early, so we throw in an effect.

That makes sense.

Can you explain why useDisposableState is different than the rest?

I'll try to re-review holistically. Great work!!! And great detective work!!!

[PatrykWalach]

HMR seems to work with iso(function Component { /*...*/ })
but not with iso('field ...')(function Component { /*...*/ })
so it would not work without babel transform 🤔

[PatrykWalach]

For no babel transform HMR will revalidate all the files upwards, but we will still rerender with stale components from the cache. To cover this case, We can add the WeakMap<Resolver, ...> layer to the cache. Then the only downside is that components will lose their current state.

If the users don't want to use babel transform but want HMR, splitting hoc would work but I'm not sure if we can recommend doing that.

const isoFoo = iso(`field Foo ...`)

export const Foo = isoFoo(function Foo(){/* ... */})

The reason this works is because react has to support builtin hoc (memo and forwardRef)

[rbalicki2]

I don't think we need to extensively support the non-babel transform case

[rbalicki2]

Can they do

export const isoFoo=  iso(`...`)(Component)

function Component() { ... }

?

[PatrykWalach]

They can't because this double function call is not recognized as hoc. Only a single call is recognized. (const Foo = memo(function Foo ...))

[PatrykWalach]

If someone doesn't use babel they still might use HMR.
They might use something like vite-swc-react and don't want to install babel. For those cases it'd be reasonable to add this extra layer of cache.