Investigate per-React-version context instance caching

[markerikson]

Per Lenz, we could try something like this:

https://github.com/apollographql/apollo-client/blob/2446acef85d5ca8af378aa11d8002b84f0e4aa38/src/react/context/ApolloContext.ts#L32-L54

[phryneas]

But using React.version as a key - we noticed that using React as a key actually doesn't work, as that is a different object in each importing file.

[TobbeLino]

Yes, latest react-redux (8.1.1) breaks our application.
We have different js-modules loaded in runtime, each with its own instance of React (same version).
And when two or more modules (using react-redux 8.1.1) run at the same time, they will ovewrite each other's context (or maybe rather they try to use the context of another React instance).
I guess the context needs to be per instance of React (not just per version).

[phryneas]

@TobbeLino could you create a reproduction for this?

[TobbeLino]

@phryneas
We are also using preact (which pretends to be React 17.0.2), so maybe that's a factor as well.

Anyway, when a 2:nd module is loaded and started (both using react-redux 8.1.1 and preact pretending to be react 17.0.2), the 2:nd instance immediately crashes on the first useSelector() with:

interactionAC.js:113 showInteraction error: TypeError: Cannot read properties of undefined (reading 'addNestedSub')
    at useSelector (useSelector.js:87:73)
    at b.ChatContainerWrapper [as constructor] (ChatContainerWrapper.js:8:33)
    at b.B [as render] (preact.module.js:1:8515)
    at L (preact.module.js:1:6225)
    at P (preact.module.js:1:2156)
    at L (preact.module.js:1:6363)
    at P (preact.module.js:1:2156)
    at L (preact.module.js:1:6363)
    at P (preact.module.js:1:2156)
    at L (preact.module.js:1:6363)

I.e. for some reason, subscription seems to be undefined on subscription.addNestedSub

Not sure if I can create something reproducible.
I'll try later tonight...

[phryneas]

@TobbeLino At this point I'd say we wait for a reproduction from you to understand this deeper.
At first glance this seems like something preact definitely should not be doing - they should at least pretend to be 17.0.2-preact-10.15.1, in the spirit of e.g. React version 18.3.0-canary-e3fb7c1de-20230621 and on first look, this might be something they should be fixing on their end rather than us trying to work around it.

In the meantime, you can create a custom context in your Preact applications as a workaround, as described here: https://react-redux.js.org/api/hooks#custom-context

[TobbeLino]

@phryneas Yeah, I know, they do this:
https://github.com/preactjs/preact/blob/523bbe93ccda2c5806284469e0e46f47e84e63b6/compat/src/index.js#L38
:) But I suppose some libraries will break if they use a custom version string...

Anyway, thanks a lot for the hint/workaround with custom context - it worked!
I'll try to investigate further and see if I can come up with something reproducible for you as well.

[phryneas]

Well, React already ships with these "weird" custom version strings, so I don't think anything would break if preact would adopt that same style... 🤔

Great that the workaround is working for you!

[TobbeLino]

@phryneas I managed to create a repo demonstrating these issues. It's a bit messy - sorry, but you should be able to reliably reproduce what I observed: https://github.com/TobbeLino/react-redux-issue

The repo can be used to test running two independent modules at the same time, both Preact (that causes a crash when "module 2" is loaded), and React 17.0.2 (which does not crash, but prints in the console: "Warning: Detected multiple renderers concurrently rendering the same context provider. This is currently unsupported.")

We will definitely use your proposed workaround (using custom context), because our apps/modules are running on customers' webpages, which can be a quite "hostile" environment 🙂. But I suspect the new default behaviour of react-redux 8.1.1 may cause very weird and hard-to-debug issues for others (from race-conditions between loaded modules order).

In my opinion (and besides any crashing and failing react/preact-modules), I think it's quite a fundamental change you have introduced here. Separate, independent react apps on the same wepage suddenly starts sharing data, and this is totally unexpected for me as a developer just using react-redux.

I'm thinking this could almost be considered to have security implications. Because if react-redux happily starts trusting objects stored in the global browser scope, what happens if another "malicious" (or buggy) app on the same webpage stores a context that react-redux in our app trusts and starts using? Or a malicious app gets access to the context exposed by our react-redux? I have no idea, but maybe that could be abused... Maybe a malicious app on the same page could inject or extract internal data from our modules (that we thought was fully isolated), like access-tokens, etc? I admit this is unlikely and far-fetched but it's a new vector that we would never have though would exist by simply using a lib like react-redux. In our apps, we try hard to really isolate them from everything else on the webpage so we do not accidentally disturb anything on the customers' page, or are affeted by their weird apps. But this feels like quite an unexpected external dependency suddenly introduced (in a minor version) 🙂

Anyway, we're good now (using a custom context), but there may be others affected by this.

[markerikson]

@TobbeLino I think you may be over-thinking this.

A context instance is essentially just another component type, albeit a type that's built-in to React. It's a marker, not an actual shared piece of data. A context instance also only has meaning inside of React's rendering logic.

[TobbeLino]

@markerikson Yeah that may very well be - I don't have deep knowledge of this 🙂 However, the issues (preact crashing and react warning) with sharing the context between totally separate instances are real I think...

[markerikson]

Yeah, we'll have to look into those. Thanks for the repro!

[TobbeLino]

@markerikson @phryneas Ok, after some more thinking (and with the risk of making a fool out of myself for not understanding this singleton-context issues your have been trying to fix with a shared context), wouldn't making sure you only share context with react-redux instances using the same (by reference) instance of React make this a lot safer and more robust?

E.g. by putting the context(s) in a WeakMap of references to the different React-instances loaded on the page.
I.e. something like this (in "/src/components/Context.ts"):

import React, { createContext } from 'react'
...

const ContextMapKey = Symbol.for('react-redux-context-map')
const gT = globalThis

function getContext() {
	let contextMap = gT[ContextMapKey];
	if (!contextMap) {
	    contextMap = new WeakMap();
	    gT[ContextMapKey] = contextMap;
	}
	let realContext = contextMap.get(React);
	if (!realContext) {
	    realContext = createContext(null);
	    if (process.env.NODE_ENV !== 'production') {
	    	realContext.displayName = 'ReactRedux'
	    }
	    contextMap.set(React, realContext);
	}
	return realContext
}

That would probably solve the preact problem we experience, the react warning when running separate react instances, and also any problems caused by preact pretending to be react 17.0.2 causing collisions between the two...

Not sure it will fix the issue you tried to resolve, but #2020 (comment) indicates that his two different instances of react-redux uses at least the same instance of react-dom. Not sure that's true by reference during runtime though, but I don't get the idea of sharing context across two completely different runtime-instances of React anyway... 🙂

[EskiMojo14]

But using React.version as a key - we noticed that using React as a key actually doesn't work, as that is a different object in each importing file.

[TobbeLino]

@EskiMojo14 Ouch, I see... However, I still don't get why different runtime instances of react should share context. As react warns: "Warning: Detected multiple renderers concurrently rendering the same context provider. This is currently unsupported."

[phryneas]

Yeah, I'm gonna put out a PR that doesn't use React or React.version out as key, but React.createContext, as that function should be stable for the same React instance over multiple importing files.

[TobbeLino]

@phryneas Thanks, the PR works beautifully!

[phryneas]

This should be solved in v8.1.2.