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

use normalizerFn when requested #11

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

pbadenski
Copy link

Useful when deep comparison is needed
Example: normalizerFn = obj => JSON.stringify(obj)

Useful when deep comparison is needed
Example: normalizerFn = obj => JSON.stringify(obj)
@thinkloop
Copy link
Owner

Thanks for the contribution. I made some updates to it here: https://github.com/thinkloop/memoizerific/tree/pbadenski-normalizer-fn

After thinking about it further, I remembered that JSON.stringify() is an unreliable way to compare objects: https://stackoverflow.com/a/26077521/1062794. And that would be the primary use-case for this addition. A straight custom compare function would probably be better.

Can you think of other uses? I hesitate to complicate the api for little good reason.

@pbadenski
Copy link
Author

This would be a primary use for me. I want to switch our project from https://github.com/medikoo/memoizee to this project. Primarily because of it's use of Map instead of Object.

I understand that there are limitations of JSON.stringify, as a user I'm willing to accept these limitations. Otherwise, ie. without a normalizerFn, I simply cannot use memoizerific.

As for custom compare fn - you'd need to give me more details. I can't see how can you combine that with use of Maps and all the performance benefits associated with them.

@thinkloop
Copy link
Owner

thinkloop commented Aug 18, 2017

I can't see how can you combine that with use of Maps and all the performance benefits associated with them.

I came to the same conclusion when I looked into it...

May I know more about your use-case?

Personally I don't have a need for custom functions like this because the objects I use for comparison I make immutable. Either I replace them completely by new objects if they are different, or they remain exactly as is, with the same reference in memory, if they are the same. When it comes time for memoization, the comparison is instant for any complexity. The comparison is shifted lower down the stack, rather than at the processing stage.

Another technique if the above is inconvenient, is to wrap the memoized function with another function that does any needed preprocessing. For example, say you wanted to memoize ajax(url, opts) where opts is an object of options. Typically this would be called with an inline object for the options like this: ajax(''https://domain.com", {timeout: 10000}) . Since a new object is created for opts on every invocation, it never gets cached. The solution is to wrap it with a function that breaks apart argumentds as needed:

function callAjax(domain, timeout) {
  return ajax(domain, {timeout: timeout});
}

And now callAjax('https://domain.com', 10000) is properly memoized. This allows for much finer grained control of exactly which arguments need to be transformed in what way. With normalizerFn you quickly start to bump into issues like blacklisting/whitelisting different properties to apply it to, etc.

Don't these seem like better solutions?

@thinkloop
Copy link
Owner

thinkloop commented Aug 19, 2017

@pbadenski
Copy link
Author

I like your solution with immutable objects, haven't thought of it.. It won't work in our case though I'm afraid. We're memoizing inside a webworker against objects that are being transferred as part of a message - hence, they will always be new copies.

I also thought of another scenario where we use custom normalizer with memoizee - memoizing against Dates normalizerFn = (date) => date.getTime().

@thinkloop
Copy link
Owner

Hey Pawel, what about the wrapper function? It works especially well with dates. Rather than passing in a date object, you pass in each part of the date as a primitive, ex: memoize(2017, 08, 21). The date is a good example of why having a custom function will be difficult to get working, since it is unlikely all params will be dates.

@pbadenski
Copy link
Author

I suppose you could use wrapper function for dates - I'm not a big fan applying it to this scenario, but point taken.

I would still need normalizerFn it for complex objects being transferred to the webworker - which is different from the dates case.

Seems that you feel much more strongly about this than I initially thought :D I'm totally fine keeping that change in a fork if you are definitely against it. :)

@thinkloop
Copy link
Owner

:-D

I try to be very judicious with APIs, there is value in being able to do 90% simply, than trying to get in the last 10% with a lot of over-engineering. The API is currently dead simple, so the bar is very low, this pull would make it significantly more complicated, so I really feel I need to weigh it. So far there are some significant negatives:

  • encourages dismissal of much better solutions for most cases (mentioned above)
  • opens door for easy mistakes, unpredictable caching
  • awkward to handle multiple arguments of different types that require different treatment
  • encourages use of JSON.stringify which is not recommended

I haven't used web workers that much, why are the aforementioned techniques not possible? The purpose of this pull request is basically to transform complex objects into primitive simple types (like an equivalent string, numbers, etc.). Why is it not possible to do the transforms manually before invoking the function?

@pbadenski
Copy link
Author

There's three "difficult" properties of objects in our scenario:

  1. they have multiple fields (up to 10-20)
  2. they are polymorphic (some actually megamorphic)
  3. only some of the object fields constitute the cache key.

First property makes it cumbersome to transform into function invocation (unrolling an object into 10 - 20 arguments). Second property, I believe, makes it impossible, unless I consider following:

fn = (obj1, obj2) => ...

memoizedFn = (obj1AsString, obj2AsString) => {
   obj1 = stringToObject(obj1AsString);
   obj2 = stringToObject(obj2AsString);
}

Unfortunately this code breaks the requirement 3.

@thinkloop
Copy link
Owner

thinkloop commented Aug 29, 2017

For 1, even though you should never using stringify(), as it will cause inconsistent results, be hard to debug (especially for new devs not aware), and possibly cause huge problems (what happens if a recursive object sneaks into your object), you can always do as you suggest for 2:

const obj1AsString = JSON.stringify(obj1);
const obj2AsString = JSON.stringify(obj2);

memoizedFn = (obj1AsString, obj2AsString) => {
   obj1 = JSON.parse(obj1AsString);
   obj2 = JSON.parse(obj2AsString);

   fn = (obj1, obj2) => ...
}

3 is a proper interesting use-case.

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 this pull request may close these issues.

2 participants