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

Drop `maxAge` #37

Closed
wants to merge 5 commits into from

Conversation

Projects
None yet
2 participants
@bfred-it
Copy link
Collaborator

commented May 11, 2019

Similarly to #36, maxAge seems just one of the ways to keep the cache clean and its implementation affects all users.

I suggest dropping the option and letting the user provide an expiry-map (by the same author of max-age-cleaner) or quickl-lru if needed, which is pretty straight forward thanks to the cache option.

Also closes #27

bfred-it added some commits May 11, 2019

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented May 13, 2019

It's provided for convenience as it's a common use-case. I don't really see the benefit of dropping it.

maxAge seems just one of the ways to keep the cache clean and its implementation affects all users.

Does it affect them in a negative way though?

I'm struggling to understand the motivation of this PR.

@bfred-it

This comment has been minimized.

Copy link
Collaborator Author

commented May 13, 2019

I'm struggling to understand the motivation of this PR.

  • the memory saving of not having to wrap all values in an object
  • module complexity, at the very least we could use expiry-map so the complexity is hidden, but...
  • adding the extra module is almost as easy as as enabling this feature, if needed at all
  • only less option to explicitly support
@sindresorhus

This comment has been minimized.

Copy link
Owner

commented May 13, 2019

the memory saving of not having to wrap all values in an object

I don't think this matters in real-world scenarios at all.

module complexity

It's like 5 lines in total.

adding the extra module is almost as easy as as enabling this feature, if needed at all

You then remove the "module complexity" and instead move it to be "docs complexity" and "user complexity".

@bfred-it

This comment has been minimized.

Copy link
Collaborator Author

commented May 13, 2019

How are the docs more complex? It just needs a and expiry-map next to the lru map mention.

It’s simply a “it doesn’t belong here” situation, just like quick-lru isn’t part of this module.

Either way it’s not a big deal 👍

const got = require('got');
const delay = require('delay');
const memGot = mem(got, {maxAge: 1000});

This comment has been minimized.

Copy link
@bfred-it

bfred-it May 14, 2019

Author Collaborator

Or I could restore this example with

+  const ExpiryMap = require('expiry-map');

+  const memGot = mem(got, {cache: new ExpiryMap(1000)});
-  const memGot = mem(got, {maxAge: 1000});
@sindresorhus

This comment has been minimized.

Copy link
Owner

commented May 14, 2019

It also complicates it if you want a custom cache that is not a Map. Then you need to manually implement maxAge yourself, or create a wrapper for your chosen data structure. I just feel that it's something many users would need, especially when using mem with HTTP requests, that it's worth having it built-in.

@bfred-it

This comment has been minimized.

Copy link
Collaborator Author

commented May 14, 2019

It also complicates it if you want a custom cache that is not a Map

That's the map implementations' fault. Outside of this module, I currently can't combine quick-lru and expiry-map for example.

I just feel that it's something many users would need

That's the reason then 👍

@bfred-it bfred-it closed this May 14, 2019

@bfred-it bfred-it deleted the bfred-it:patch-3 branch May 14, 2019

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented May 14, 2019

Outside of this module, I currently can't combine quick-lru and expiry-map for example.

I'm happy to support passing in a custom map to quick-lru. Open an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.