Immer: A more robust condition for 'setUseProxies'

[ilatif]

🚀 Feature Proposal

Currently immer is checking whether to useProxies or not based on the following condition:

typeof Proxy !== "undefined" && typeof Proxy.revocable !== "undefined" && typeof Reflect !== "undefined"

This condition could be improved such that it also checks whether Proxy is native or not in-order to overcome unexpected errors which might arise when someone is using both immer and proxy-polyfill.

Motivation

I recently came across an issue on IE11 where despite my assumption that immer would work on, I was getting Proxy polyfill does not support trap 'has' error. On investigation I found that this issue is coming an external library which is using proxy-polyfill which is populating/assigning its polyfill globally to window object and since my app is an Angular app (Angular 8) and which has Reflect polyfill added, the condition for useProxies was evaluating to true which caused immer to use modernProxy instead of the legacyProxy and caused the aforementioned error. We can maybe update the above check to something like the following:

typeof Proxy !== "undefined" && Proxy.toString() === 'function Proxy() { [native code] }' && typeof Proxy.revocable !== "undefined" && typeof Reflect !== "undefined"

In above, we are confirming whether Proxy conforms to the native version provided by the browsers before proceeding forward.

Doing this will eliminate unexpected behaviours if some Proxy polyfill is used.

_P.S I used setUseProxies from immer to explicitly override the behaviour, but I guess we can handle this more appropriately internally._

Can this be sovled in user-land code?

Yes

[ilatif]

I am happy to open a PR, if you think this ticket makes sense 😊.

[oriSomething]

I think real feature detection is better, something like in https://kangax.github.io/compat-table/es6/

For example:

eval(`+function test() {
var passed = false;
new Proxy({},{});
// A property cannot be reported as deleted, if it exists as a non-configurable
// own property of the target object.
var proxied = {};
Object.defineProperty(proxied, "foo", { value: 2, writable: true, enumerable: true });
try {
  delete new Proxy(proxied, {
    deleteProperty: function () {
      passed = true;
      return true;
    }
  }).foo;
  return false;
} catch(e) {}
return passed;
}()`);

But I think in one application it make no sense to use both "proxy" and "es5" modes, it just can make more unwanted bugs that hard to find

[oriSomething]

Okay, i've wrapped in not good... but the idea is clear

[ilatif]

It's not about using Proxy and es5 together. It's about immer picking appropriate implementation of Proxy. As per my understanding of code, immer determines whether to use available Proxy implementation or use its own implementation which isn't a Proxy polyfill per say. It is just that in non-supporting browsers there might be a Proxy polyfill included which could be used to target a specific scenario and might not work well with immer. This problem can simply be avoided by making sure that immer would only use native Proxy implementation and in-case it's not present, it will fallback to its own implementation instead of a custom Proxy polyfill which might not work well with immer.

[mweststrate]

My 2 cents: I think we shouldn't be reasoning about whether a polyfill is correct or not, if one polyfill is broken, and the other one isn't, how can we decide whether to use a polyfill or not? Run a full es test suite during startup of the module (since we basically use every Proxy feature in the book)? That doesn't really sound like a plan. I think it is the responsibility of the person that adds the polyfill, to make sure it is a properly working one and if it isn't, disable the proxy usage in Immer.

[ilatif]

@mweststrate Thanks for commenting.

You are absolutely right about it being the responsibility of the person to properly handle Proxy usage in immer. It is just that we for sure know that browser's native Proxy is the most accurate and reliable implementation than others and if there is some other implementation besides that, immer could fallback to its own implementation. Asking immer to disable the usage of unwanted Proxy implementations is pretty straight-forward but still vary depending on framework-to-framework. I had to inject my reducers using Dependency Injection in my Angular app to be able to decide and disable unwanted Proxy usage.

In-short, while checking Proxy, if we could just check if that's the native one (using the way I mentioned in the main comment, there might be other ways as well) only then use it and in other scenarios fallback to the internal implementation. Non-native implementations/polyfills tend to be slower than the native one, so its better to use immer's internal implementation if that's the case.

_P.S I am thinking of writing a blogpost on how Google's proxy-polyfill conflicted with immer and how to fix in Angular app 😊. I thought I should create the ticket so that people know I am pretty serious about getting this fixed 😛._

[aleclarson]

:tada: This issue has been resolved in version 7.0.0 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket:

[ilatif]

@aleclarson Awesome! That's huge 🚀.