dai-shi / proxy-compare

Compare two objects using accessed properties with Proxy
https://www.npmjs.com/package/proxy-compare
MIT License
283 stars 18 forks source link

alternative of Proxy #40

Closed janjangao closed 2 years ago

janjangao commented 2 years ago

hi, I am trying to use lib proxy-memoize in our website to replace reselect, but our website serves customers, so need be compatible with as many browsers as possible, but proxy-polyfill can't work with this lib, since the polyfill only support a limited scope traps, not include has, deleteProperty and so on.

I also take a look of immerjs to check how it works in non-proxy environment, it use Object.defineProperty instead, so what I think is:

  1. add a condition setUseProxies to disable using Proxy.
  2. have a backup Object.defineProperty implementation, for this scenario, only support limit recording method.
    • record traps get and set
    • getOwnPropertyDescriptor and ownKeys are rarely used and also reasonable if not record them.
    • has and deleteProperty are big problems, only hope the developers notice to don't use them.
      • obj['a'] instead of 'a' in obj
      • const { ...newObj, a } = obj instead of delete obj.a

then I think proxy-memoize can have a widely usage scenarios, hope can as a default selectorCreator be putted in the redux-toolkit one day ^_^

dai-shi commented 2 years ago

Hi, thanks for opening up a discussion. tbh, I don't want to add non-proxy capability to this lib. But, I do have interests how to make it work with polyfills and what would be the limitations. Essentially, I think it's a doc issue, but if there's room for another polyfill, we can try creating one. It would be technically the same thing. Have you looked into the proxy-polyfill implementation? Not only in and deleteProperty are difficulties, but also adding a new property on demand.

Somewhat related issue: https://github.com/dai-shi/react-tracked/issues/6

btw, proxy-memoize uses WeakMaps. You would also need a polyfill, but then it will probably leak memory...

janjangao commented 2 years ago

Not only in and deleteProperty are difficulties, but also adding a new property on demand.

I see I see

You would also need a polyfill, but then it will probably leak memory...

ah, got it, I wasn't aware of this before

I also have a new proposal to avoid the problems(Proxy, WeakMaps).

it's more concerned with proxy-memoize, let us limit the usage scenarios, if have a lib only need works with redux selector, it only care the property reading, not mutation, and at least have one cache.

As I roughly read the source code of these two libs, the WeakMaps is mainly used for weakly storing the object type and properties path, so according to the react scenario I talked about:

I understand proxy-memoize and proxy-compare are pure tools working for more common situation, but can have a variation version for redux usage, maybe named selector-memoize :).

I knew proxy-memoize from redux official website, it's highly recommended as an alternative with reselect, I do hope can have a more suitable variation version for redux.

dai-shi commented 2 years ago

Actually, the WeakMap cache in proxy-memoize would work best with redux state. It provides infinite cache size with no memory leaks.

So, your proposition is selector-memoize which uses no WeakMaps and depends on Object.defineProperty instead of Proxy. While I agree it's interesting to try, it will have more limitations. I wonder if it would still be recommended as an alternative to reselect. My gut feeling is it has so many limitations that replacing reselect doesn't make much sense.

janjangao commented 2 years ago

I think still can have proxy and weakmap, add a toggle to open es5 support as a workaround, like enableES5 in immerjs, decided by the developer, if they known the limitations.

dai-shi commented 2 years ago

I would like to know if that works in your case practically. Technically enableES5 should be the same as using polyfills. Would you like to try polyfills in your project?

dai-shi commented 2 years ago

Just looking at this: https://github.com/polygonplanet/weakmap-polyfill

but then it will probably leak memory...

No, it doesn't leak memory.

dai-shi commented 2 years ago

https://github.com/zloirock/core-js oh, okay, core-js has polyfills for weakmaps and symbols. https://github.com/zloirock/core-js#missing-polyfills only missing is proxy-polyfill, I guess.

janjangao commented 2 years ago

Would you like to try polyfills in your project?

yes, that's my original plan, but follow this issue, global proxy-polyfill seems can't work very well with immerjs, and proxy-compare also use trap has, I thought also will throw error Proxy polyfill does not support trap 'has' when use in operator, I want it can just work instead of throw error.

No, it doesn't leak memory.

that's great, so the WeakMap isn't the problem.

dai-shi commented 2 years ago

https://github.com/GoogleChrome/proxy-polyfill/blob/9dc3aa4ac356308e337f85bb9e671438b4da0ece/src/proxy.js#L100-L106

Okay, this is very unfortunate. I think polyfill shouldn't do this, but understandable.

dai-shi commented 2 years ago

Please try #41. https://ci.codesandbox.io/status/dai-shi/proxy-compare/pr/41 👈 See "Local Install Instructions"

janjangao commented 2 years ago

Wow, you are so productive!!, I saw your workaround, there is little gap from my expectation, I have to handle the compatibility with immerjs if I use proxy-polyfill, it doesn't advice to use proxy polyfill, maybe I can make my own polyfill to suit these two, immerjs only use Proxy.revocable, you use constructor, that's can be the trick. Or I can implement my proposition myself(I still think it's a good idea ^_^)

Anyway, your change for this lib is perfect, thanks for your time and effort, I think can close this issue.

dai-shi commented 2 years ago

I have to handle the compatibility with immerjs if I use proxy-polyfill

Hmm, that doesn't sound ideal. I wonder if I can make it so that you can optionally use the polyfill: https://github.com/GoogleChrome/proxy-polyfill#to-consume-the-polyfill-as-a-function

I can implement my proposition myself(I still think it's a good idea ^_^)

Yeah, please go ahead.

I think can close this issue.

I actually need your feedback, if #41 really solves your use case.

dai-shi commented 2 years ago

Please check the new commit in #41.

janjangao commented 2 years ago

https://github.com/GoogleChrome/proxy-polyfill#to-consume-the-polyfill-as-a-function

ah, I also take a look of this

I actually need your feedback, if https://github.com/dai-shi/proxy-compare/pull/41 really solves your use case.

sure sure, no problem, any question I will also put in this, I am developing a redux tool named create-hooks-slice, currently it's highly dependent on immerjs and proxy-memoize. and I want to apply it in my daily work, unfortunately, the company I worked is serving apps for large of customers, so browser compatibility is a hard requirement.

Please check the new commit in https://github.com/dai-shi/proxy-compare/pull/41.

wow, that's fascinating, hope this replaceNewProxy can also export from proxy-memoize, and release new version, that's very exciting, I think I can simple wrap these libs without my own implementation. thanks your great job.

dai-shi commented 2 years ago

Once you confirm #41 works as expected, I will cut a new release and release proxy-memoize too. You know what, I can create a new proxy-memoize PR, so that you can try it easily.

dai-shi commented 2 years ago

See https://github.com/dai-shi/proxy-memoize/pull/48

janjangao commented 2 years ago

done the MR approval!

janjangao commented 2 years ago

See https://github.com/dai-shi/proxy-memoize/pull/48

got it, thanks