-
Notifications
You must be signed in to change notification settings - Fork 86
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
serializing class instances for 2+ arguments #9
Comments
Thanks for the well written bug report. The trick here is how to make it as fast as possible. |
happy to help. I understand that the speed is the main priority, but I'd consider it a rather serious flaw - it should be at least documented :) |
Another potential issue: since you're using instances as cache keys, you potentially keep the instances from being GC'd and hogging the memory - just something to think about :) |
Totally true. Maybe |
sure, but here's a twist: how will you serialize and compare the |
You could create a special cache for a calls with |
Since this lib is very focused on being fast, I'm starting to think the only way to maintain speed is to expose some complexity to the user. Like, if you want to memoize a function that will receive non-primitive arguments, do like: const memoized = memoize(fn, {
optimizeFor: 'object'
}) For primitive types: const memoized = memoize(fn, {
optimizeFor: 'primitive'
}) Or if you (as a client of the lib) don't want to think about details, just use a common solution: const memoized = memoize(fn) This way the Thoughts? |
I'd just handle the one argument scenario and document the complexity without implementing. current solution will work for most of the cases, so it should be good. However, it needs to be documented/mentioned :) You could provide the proposed solution as well, given that it's documented too :) WDYT? |
Sorry intruding on this, but I would love to have it fixed. So, what if we had something like:
Then, |
reading through the code, I found this:
given that the default serializer uses
JSON.stringify()
you'll potentially get wrong cache-hits for any memoized function calls that accept class instances (that don't "expose" all their props) as their arguments (given that you provide more than one argument).
example
The text was updated successfully, but these errors were encountered: