Bugs when surrounding HOCs that hoist statics

[helixbass]

On the current project, we ran into mysterious bugs when placing two react-apollo graphql() HOCs in the same compose() chain. Turned out that because each was followed by a withProps() (ie compose(graphql(), withProps(), graphql(), withProps())) and graphql() uses hoist-non-react-statics to hoist statics, bugs were because graphql() was hoisting the "internal" symbol statics that recompact uses to identify other components as fellow "compactable" Recompact HOCs, so withProps() thought that graphql() was another Recompact HOC that it could condense or whatever (at least this is my understanding)

So my solution was to fork recompact to export the symbols it uses as keys for static properties (opened #95) and fork react-apollo to accept a blacklist of keys not to hoist (which can then be passed the exported recompact symbols)

However, then this week ran into the same issue using a different HOC (withNavigation() from react-navigation) which also hoists statics using hoist-non-react-statics. So hoping there's a better approach than trying to get various third-party HOCs to accept a hoist blacklist. This feels at this point like a flaw in the way recompact identifies other recompact HOCs, since it's considered best practice for HOCs to hoist statics (which to my understanding then causes bugs whenever it sits between two recompact HOCs), but not sure if there's another mechanism besides statics that recompact could use to identify other recompact HOCs?

[matthieuprat]

Thank you for the detailed explanation.

Apparently, react-proxy faced a similar issue and went for a WeakMap (gaearon/react-proxy#45). Looks like a good path forward.

We'll need to provide a polyfill for older browsers, perhaps something in a similar vein to https://github.com/gaearon/react-proxy/blob/v2.0.8/src/createClassProxy.js#L33-L40 which is more lightweight than core-js's WeakMap and safer (see gaearon/react-proxy#50).

Thoughts?

[helixbass]

@matthieuprat sure, that does look like basically the same problem faced by react-proxy

My understanding from looking over those react-proxy links is that they tried to use a WeakMap but then switched to using a list to avoid the issues encountered with core-js's WeakMap. Then there is open PR gaearon/react-proxy#62 which may provide more insight - suggests alternative WeakMap polyfills I guess. The advantage to using WeakMap is that (a) references to created HOCs (in keys of WeakMap) are weak and so can be garbage-collected (this comment says that using WeakMap "wasn't paramount in our case" for react-proxy, is recompact different?) (b) lookup time is static (this is referenced in the title of gaearon/react-proxy#62)?

Either way (WeakMap or list) the idea is to use an external module-level data structure to keep track of all recompact HOCs rather than via a static property set on the HOCs themselves, right?

So for recompact would you need two tracking WeakMaps/lists corresponding to compactable and MAPPERS_INFO symbols? So eg isMapperComponent() would lookup component in mapper-tracking data structure and instead of setting MAPPERS_INFO as static when creating class in createComponentFromMappers(), you'd register/store the created class in mapper-tracking data structure (before returning it)?

[matthieuprat]

Exactly.

this comment says that using WeakMap "wasn't paramount in our case" for react-proxy, is recompact different?

Yes, I think it is, in that unlike react-proxy, recompact is intended to be used in production, which means the performance and memory requirements are not the same.

So, it's worth trying to use a WeakMap as the backing data structure if it is available and use a list as a fallback only if it is not available.

Are you interested in working on this? Otherwise, I might be able to free up some time this week to tackle this.

[helixbass]

@matthieuprat sure I'll try and get in a PR sometime this week, thanks