-
Notifications
You must be signed in to change notification settings - Fork 4
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
Compatibility issues with mobx-react #4
Comments
I've come up with a userland-only fix for this issue. However, brace yourself as it is not pretty. Behold my masterpiece of clean code: 😆 export function ClassHooks(targetClass: Function) {
const componentWillMount_orig = targetClass.prototype.componentWillMount;
targetClass.prototype.componentWillMount = function() {
const MAGIC_STACKS = GetMagicStackSymbol(this);
// by initializing comp[MAGIC_STACKS] ahead of time, we keep react-class-hooks from patching this.render
if (!this[MAGIC_STACKS]) this[MAGIC_STACKS] = {};
if (componentWillMount_orig) return componentWillMount_orig.apply(this, arguments);
};
const render_orig = targetClass.prototype.render;
// note our patching Class.render, not instance.render -- this is compatible with mobx-react
targetClass.prototype.render = function() {
const MAGIC_STACKS = GetMagicStackSymbol(this);
// apply the stack-resetting functionality normally done in the on-instance patched this.render
Object.getOwnPropertySymbols(this[MAGIC_STACKS]).forEach(k=>{
this[MAGIC_STACKS][k] = 0;
});
return render_orig.apply(this, arguments);
};
}
let magicStackSymbol_cached: Symbol;
export function GetMagicStackSymbol(comp: Component) {
if (magicStackSymbol_cached == null) {
const instanceKey = React.version.indexOf("16") === 0 ? "stateNode" : "_instance";
const ReactInternals = React["__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED"];
const compBeingRendered_real = ReactInternals.ReactCurrentOwner.current;
const compBeingRendered_fake = {render: ()=>({})};
ReactInternals.ReactCurrentOwner.current = {[instanceKey]: compBeingRendered_fake};
useRef(); // this triggers react-universal-hooks to attach data to the "comp being rendered" (fake object above)
//useClassRef(); // variant, if only using the underlying react-class-hooks library
ReactInternals.ReactCurrentOwner.current = compBeingRendered_real;
// now we can obtain the secret magic-stacks symbol, by iterating the symbols on compBeingRendered_fake
const symbols = Object.getOwnPropertySymbols(compBeingRendered_fake);
const magicStackSymbol = symbols.find(a=>a.toString() == "Symbol(magicStacks)");
magicStackSymbol_cached = magicStackSymbol;
}
return magicStackSymbol_cached as any; // needed for ts to allow as index
} Usage:
Note that about half of the code is just to be able to obtain the MAGIC_STACKS symbol created within react-class-hooks. If react-class-hooks were to expose the symbol in its exports, the fix above would be much shorter to implement. Alternative ways to get the MAGIC_STACKS symbol:
|
Here are some ideas for how to fix the issue within the library directly, removing the need for the above hack: 1) Provide an official decorator to modify the componentWillMount and render functions, similar to that seen above. It, of course, would already have access to the MAGIC_STACKS symbol, reducing the complexity a lot. Actually, it wouldn't need to override the class Anyway, if this route is taken, perhaps we could make it optional to use the decorator, as something you only need to do if using the library with mobx-react; elsewhere, one could continue using the current implicit way. (where you don't need a class decorator) EDIT: You could even wrap the 2) There are probably others, but I've spent too much time on this already, so I'm leaving further ideas up to whoever else hits this issue. XD |
Oh you gotta be kidding me, I bet this was the real source of that crash bug. I was using class hooks in a component decorated by mobx-react! Guess I better apply this fix to those instances. |
hi, i will release a fix asap later this night. even if mobx-react think that it's the only one that can swap this.render LOL i'm thinking about runtime patching mobx, so you have to do nothing. stay tuned! 🎉 |
|
This library appears to have conflicts with mobx-react.
Here is the thread where I discovered the issue, along with analysis of the cause: mobxjs/mobx-react#797
Summary: Because react-class-hooks replaces
this.render
after mobx-react has already created a reaction object (attached to the original render function), whenever MobX detects a change, it only calls the original render function -- not the wrapper that react-class-hooks added late (onto the instance).What problems does it cause?
this.render
override by react-class-hooks would not be called, meaning the stack counter would not be reset, leading to it just keeping on adding more cells instead of reading from existing ones.The text was updated successfully, but these errors were encountered: