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

feat: expireDuration feature added #704

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

zaferatli
Copy link

@zaferatli zaferatli commented Jul 16, 2024

related enhancement #615

@mrousavy
Copy link
Owner

Hey - thank you for your PR, this is great!

One small change - can you change enableAutoKeyExpiration to be a getter + setter boolean property, instead of a function? Thanks!

And also, make sure to run lint (yarn check-all) to format the C++ code so the CI is happy.

@zaferatli
Copy link
Author

i change enableAutoKeyExpiration function to getter + setter without boolean property because this function takes number parameter.

i use -1 pointer for disableAutoKeyExpiration, its came to me more easy-to-use but if want to change i could

@mrousavy
Copy link
Owner

ohhh - I'm sorry I misunderstood that then. I thought it was a simple boolean!

Hmm, let me think about this, not sure if I want this to be a prop or a function now..

I'll get back to you! :)

@zaferatli
Copy link
Author

have you had a chance to review it? i think we can use function its would be more easy-to-use

@mrousavy
Copy link
Owner

Hey - so two thoughts:

  1. I don't like the way every set function now has an extra branch. To me, it looks much more complex than before. I want to avoid that. If you take a look at what MMKV itself does internally, is it just passes the currently configured key duration as a default value. So I think we can also do that if we can access that maybe?
  2. If it is a property, it needs to be number | undefined, as in either a number or not set. But I think a function makes more sense.

I'm unfortunately still not happy about both solutions, I think this adds a lot of unnecessary complexity and I don't really get what you would need that for? Why do you need your keys to expire? Can you give me some usage examples?

@zaferatli
Copy link
Author

i just saw related issue, and i've wanted to do contributed it and if u want to put behind until we need its ok for me

@mrousavy
Copy link
Owner

Thank you - I really appreciate the contribution - but I want to avoid merging something that overcomplicates the codebase, because that makes it harder to contribute or improve the project in the future.

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