nx-js / observer-util

Transparent reactivity with 100% language coverage. Made with ❤️ and ES6 Proxies.
MIT License
1.2k stars 94 forks source link

TypeError: Illegal invocation #21

Closed Lokua closed 6 years ago

Lokua commented 6 years ago

Hi. I'm experiencing an issue when using react-easy-state, however the TypeError is coming from this lib so I thought it best to report issue here. This may not be a bug but rather a lack in my understanding of how proxies work or what limitations they may have, or perhaps the issue lies in the Tone.js library classes I'm trying to proxy, in any case here is an example that I think will illustrate the issue better than I can explain:

import React from "react";
import { render } from "react-dom";
import { store, view } from "react-easy-state";
import Tone from "tone";

const osc1 = new Tone.Oscillator();
const osc2 = new Tone.Oscillator();

const proxy = new Proxy(osc1, {
  get(...args) {
    return Reflect.get(...args);
  }
});

// works
console.log(proxy.frequency.value); // => 440

const oscStore = store(osc2);

// throws TypeError: Illegal invovation
const App = view(() => <div>{oscStore.frequency.value}</div>);

render(<App />, document.getElementById("root"));

codesandbox

In the example would expect oscStore to behave exactly like osc2 object or even the proxy object.

solkimicreb commented 6 years ago

Hi!

Could you provide a full error message and stack trace? It is probably a one line fix, which I can do in the aftetnoon. I will give you some explanation in a few hours when my laptop will be around. I hate typing on phones 😀

Thanks for the issue

Lokua commented 6 years ago

Sure thing:

stacktrace:

Uncaught TypeError: Illegal invocation
    at Object.get (<anonymous>)
    at Object.get (es.es5.js:307)
    at Proxy.get (Tone.js:3457)
    at Object.get (<anonymous>)
    at Object.get (es.es5.js:307)
    at index.js:27
    at ReactiveHOC.render (es.es5.js:38)
    at runAsReaction (es.es5.js:87)
    at ReactiveHOC.reaction [as render] (es.es5.js:141)
    at finishClassComponent (react-dom.development.js:8437)
    at updateClassComponent (react-dom.development.js:8414)
    at beginWork (react-dom.development.js:8957)
    at performUnitOfWork (react-dom.development.js:11005)
    at workLoop (react-dom.development.js:11069)
    at HTMLUnknownElement.callCallback (react-dom.development.js:104)
    at Object.invokeGuardedCallbackDev (react-dom.development.js:142)
    at invokeGuardedCallback (react-dom.development.js:191)
    at renderRoot (react-dom.development.js:11143)
    at performWorkOnRoot (react-dom.development.js:11869)
    at performWork (react-dom.development.js:11795)
    at performSyncWork (react-dom.development.js:11773)
    at requestWork (react-dom.development.js:11696)
    at scheduleWorkImpl (react-dom.development.js:11546)
    at scheduleWork (react-dom.development.js:11503)
    at scheduleRootUpdate (react-dom.development.js:12122)
    at updateContainerAtExpirationTime (react-dom.development.js:12150)
    at Object.updateContainer (react-dom.development.js:12168)
    at ReactRoot../node_modules/react-dom/cjs/react-dom.development.js.ReactRoot.render (react-dom.development.js:15464)
    at react-dom.development.js:15883
    at Object.unbatchedUpdates (react-dom.development.js:11979)
    at legacyRenderSubtreeIntoContainer (react-dom.development.js:15879)
    at render (react-dom.development.js:15947)
    at Object../src/index.js (index.js:29)
    at __webpack_require__ (bootstrap 873b926e025682e40beb:678)
    at fn (bootstrap 873b926e025682e40beb:88)
    at Object.0 (index.js:29)
    at __webpack_require__ (bootstrap 873b926e025682e40beb:678)
    at bootstrap 873b926e025682e40beb:724
    at bootstrap 873b926e025682e40beb:724

Source here at https://github.com/nx-js/observer-util/blob/master/src/handlers.js#L13 from calling this guy: https://github.com/Tonejs/Tone.js/blob/master/Tone/core/Param.js#L82

solkimicreb commented 6 years ago

This is an issue with the interaction of Proxies and built-in objects. Illegal Invocation errors are thrown when a function is invoked with the wrong this. When you try to Proxy wrap a built-in object, the this for its methods becomes a Proxy and it throws this error. This means Built-ins can not really be wrapped with Proxies.

This lib handles built-ins differently, to work around this. It monkey patches built-in objects instead of Proxy wrapping them. In your case Tone.js is wrapping the AudioParam web built-in, which I forgot to handle specially. You could not reproduce with your proxy test, since you wrapped the wrong object.

A lot happens behind the scenes in this case. The frequency.value prop is a getter, and the getter function gets values from a hidden AudioParam instance. This lib auto wraps the hidden AudioParam instance with a Proxy (since it is used inside the render) and this is where the error is thrown.

I can quick fix this by including AudioParams in the built-in list. In the long term I will figure out something more elegant 🙂 More and more people store really complex data in the stores, which I did not expect. I will try my best to adapt to the use cases.

Thanks for the issue, it is very informative. I will let you know when the fix is out (it might take one more day).

Lokua commented 6 years ago

Thanks for the detailed explanation! I can see how keeping a static list of builtins could get tedious - perhaps exposing a registerBuiltin fn would help take that off of your plate?

In any case thanks again for your time and the tools. It is pretty crazy to store things like this I admit, but in the case of audio it starts getting really tedious to have to keep basically a separate set of "ui" vars and have to update both - something I've had a pretty hard time doing with mobx / redux - even vanilla react. The fact that I could even get this far with react-easy-state is pretty impressive, and the idea of being able to just show an audio param value, move a slider and voila, ui + param updated!? That's pretty powerful stuff!

solkimicreb commented 6 years ago

The fix is ready and it works (reacts to audio changes). In the end I added a general built-in detection. I check if the object has a constructor that is available in the global Object and I have a whitelist for supported globals (like Array, Object and ES6 collections). This might have other caveats, but it seems like a better approach. I will merge it in and release today, I was testing it with another delicate change in the past days.

solkimicreb commented 6 years ago

I released Observer Util v4.1.2 with a fix for this. It took longer than expected, I was chasing none existent performance issues 😄

Easy State did not get a new version, I let semver do it's job with transient dependencies. You might need to update your package-lock, if you use any. Let me know if you have issues with the reactivity (it won't throw illegal invocation errors anymore, but it might not always react as you expect it to - in case of WebAudio built-ins).

Thanks for the issue, closing it now 🙂

Lokua commented 6 years ago

Just reporting back, so far I am having no issues with reactivity. Thanks again.