RangeError when passing an observable HTMLElement to toJS

[maxdeviant]

When you pass an observable HTMLElement to toJS, a RangeError occurs:

mobx.js:427 Uncaught (in promise) RangeError: Maximum call stack size exceeded(…)

[maxdeviant]

Working on a fix for this, but having a hard time writing tests, since there is no DOM in the test runner.

[andykog]

@maxdeviant, what do you mean by “observable HTMLElement”? Please, don't say that you are doing something like extendObservable(myDiv, {}) :-)

Working on a fix for this, but having a hard time writing tests, since there is no DOM in the test runner.

You can run it in chrome like this: npm run test-browser-chrome, but note that mobx should not be tight to DOM.

[andykog]

Strange, that this error occurs, it have to be prevented by __alreadySeen check.

[maxdeviant]

@andykog Well, my original case was something like this:

class MyStore {
    @observable
    public images = asMap<HTMLImageElement[]>({
        123: new Image()
    });
}

// somewhere else in my code
console.log(toJS(myStoreInstance.images));
// throws the RangeError

What I am really interested in is when entries are added to/removed from the map, not necessarily observing the element itself.

[maxdeviant]

note that mobx should not be tight to DOM.

I noticed that when I tried a simple instanceof HTMLElement check and it said HTMLElement was undefined 😅

It appears that there are ways to check for an HTML element without instanceof, like looking for certain properties, etc.

[andykog]

Well instanceof check can be made safely like this:

var checkIfDomNode = (typeof document === 'object' && document && document.createElement && typeof HTMLElement === 'function' && typeof Node === 'function')
  ? s => s instanceof Node
  : () => false;

[maxdeviant]

Ah, good point. I had it done like this:

function isHTMLElement(source) {
    try {
        return source instanceof HTMLElement;
    } catch (err) {
        return (
            source !== null &&
            typeof source === "object" &&
            source.nodeType === 1 &&
            typeof source.style === "object" &&
            typeof source.ownerDocument === "object"
        );
    }
}

Also, is there a way to add a test that only runs when executed in a browser? It doesn't appear that process.env.BROWSER (or something equivalent) is being set anywhere when running the test suite.

[andykog]

My approach is a bit more performant :-)

Also, is there a way to add a test that only runs when executed in a browser? It doesn't appear that NODE_ENV is being set anywhere when running the test suite.

Nope, but you can detect DOM the same way:

t.test('...', function(t) {
 if (typeof document === 'object') {
   toJS(document.createElement('img'));
 }
 t.end();
} 

[mweststrate]

I'm not entirely sure about this PR;

1. as @andykog indicated, the alreadySeen mechanism should prevent this behavior, maybe there is a bug in there
2. this fix is not really scalable, generic, as there is potentially an unlimited amount of concepts in JavaScript that should not be serialized (for example: Promises)
3. This makes me wonder whether the current behavior of toJS is actually desired. Probably it should only process plain objects / arrays and collections known to MobX, but not other objects that have a prototype? (Or maybe just their observable properties and nothing more)

[andykog]

@mweststrate, this bothers me in current toJS behaviour as well, but to change it we have to at least somehow mark classes with properties decorated with @observable

[mweststrate]

@andykog they return true in isObservableObject. From there on we could iterate the observable, non computed properties as stored in the $mobx administration object

[maxdeviant]

@mweststrate You are correct in that my PR is not really generic, since it grew out of an issue that we were encountering in our codebase.

So if there is a better solution that can be achieved that fixes the issue, I am all for that.

[mweststrate]

Closing this issue as it has been solved in 2.7.0. Feel free to re-open if this isn't the case.