Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

alternative of Proxy #40

Closed
janjangao opened this issue Jun 20, 2022 · 18 comments · Fixed by #41
Closed

alternative of Proxy #40

janjangao opened this issue Jun 20, 2022 · 18 comments · Fixed by #41

Comments

@janjangao
Copy link

janjangao commented Jun 20, 2022

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
Copy link
Owner

dai-shi commented Jun 20, 2022

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: dai-shi/react-tracked#6

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

@janjangao
Copy link
Author

janjangao commented Jun 20, 2022

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:

  • If only care data reading, only trap get is needed, so Object.defineProperty can instead of Proxy
  • if always have one cache, we can store the properties path info with the cache, no need to use WeakMaps to garbage collection.

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
Copy link
Owner

dai-shi commented Jun 20, 2022

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
Copy link
Author

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
Copy link
Owner

dai-shi commented Jun 21, 2022

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
Copy link
Owner

dai-shi commented Jun 21, 2022

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
Copy link
Owner

dai-shi commented Jun 21, 2022

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
Copy link
Author

janjangao commented Jun 21, 2022

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
Copy link
Owner

dai-shi commented Jun 21, 2022

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
Copy link
Owner

dai-shi commented Jun 21, 2022

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

@janjangao
Copy link
Author

janjangao commented Jun 21, 2022

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
Copy link
Owner

dai-shi commented Jun 21, 2022

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
Copy link
Owner

dai-shi commented Jun 21, 2022

Please check the new commit in #41.

@janjangao
Copy link
Author

janjangao commented Jun 21, 2022

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 #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 #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
Copy link
Owner

dai-shi commented Jun 21, 2022

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
Copy link
Owner

dai-shi commented Jun 21, 2022

See dai-shi/proxy-memoize#48

@janjangao
Copy link
Author

done the MR approval!

@janjangao
Copy link
Author

janjangao commented Jun 21, 2022

See dai-shi/proxy-memoize#48

got it, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants