-
-
Notifications
You must be signed in to change notification settings - Fork 842
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
A more robust condition for 'setUseProxies' #514
Comments
I am happy to open a PR, if you think this ticket makes sense 馃槉. |
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 |
Okay, i've wrapped in not good... but the idea is clear |
It's not about using |
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. |
@mweststrate Thanks for commenting. You are absolutely right about it being the responsibility of the person to properly handle In-short, while checking P.S I am thinking of writing a blogpost on how Google's |
馃帀 This issue has been resolved in version 7.0.0 馃帀 The release is available on: Your semantic-release bot 馃摝馃殌 |
@aleclarson Awesome! That's huge 馃殌. |
I saw this issue happened again on Safari 14.0.1, throw by immer.
We are using immer 8.0.1. Any idea? |
@Justin-CATCH Do you directly or indirectly
|
as mentioned before proxy-polyfill makes the feature detection pass, but then delivers a broken implementation. So there is little we can do here. proxy-polyfill itself is simply broken (conceptually it is even impossible to polyfill Proxy correctly). So either just remove proxy-polyfill, or explicitly configure Immer to not use proxies. |
Couldn't agree more 馃憤 .
I unfortunately had to go with the latter one because |
Thanks all. What is the implication of not using proxy for immer? |
@Justin-CATCH I think I read somewhere in docs that it'd have performance implications. But in all honesty, I didn't see anything performing slow in my app. |
馃殌 Feature Proposal
Currently
immer
is checking whether touseProxies
or not based on the following condition: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 bothimmer
andproxy-polyfill
.Motivation
I recently came across an issue on IE11 where despite my assumption that
immer
would work on, I was gettingProxy polyfill does not support trap 'has'
error. On investigation I found that this issue is coming an external library which is usingproxy-polyfill
which is populating/assigning its polyfill globally towindow
object and since my app is an Angular app (Angular 8) and which hasReflect
polyfill added, the condition foruseProxies
was evaluating totrue
which causedimmer
to usemodernProxy
instead of thelegacyProxy
and caused the aforementioned error. We can maybe update the above check to something like the following: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
fromimmer
to explicitly override the behaviour, but I guess we can handle this more appropriately internally.Can this be sovled in user-land code?
Yes
The text was updated successfully, but these errors were encountered: