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

add a .has() method #131 #132

Merged
merged 2 commits into from
Jul 26, 2019
Merged

add a .has() method #131 #132

merged 2 commits into from
Jul 26, 2019

Conversation

regevbr
Copy link
Contributor

@regevbr regevbr commented Jul 24, 2019

add a .has() method #131

@regevbr
Copy link
Contributor Author

regevbr commented Jul 24, 2019

@erdii can you please code review?
It seems that the changes you made to dependencies in b208726 has messed up the npm install process on old node versions and I'm not sure how to solve it besides force upgrading npm to v3 in those tests

@erdii
Copy link
Member

erdii commented Jul 25, 2019

@regevbr yes there are breaking changes in the pipeline. we will be dropping support for node < 6.x (maybe even 6.x) in the next major release.
I should remove those old node versions from the tests... 😅

Code is nice and should work, but the call to exists (to check if the key already expired and was not cleaned up yet) is missing.

PS: CoffeeScript has an existential operator: instead of x != null you can write x?

@erdii erdii self-assigned this Jul 25, 2019
@erdii erdii self-requested a review July 25, 2019 10:34
@regevbr
Copy link
Contributor Author

regevbr commented Jul 25, 2019

@erdii thanks! that is a good catch - I fixed the code and added tests to that use case.

About the deprecation, please keep support for node 6 as I'm stuck with it sadly and not in a position to upgrade.

@erdii
Copy link
Member

erdii commented Jul 26, 2019

@regevbr Ok I'll bring that up when we'll talk about compatibility! Thanks for your work here!

@erdii erdii merged commit 550d979 into node-cache:master Jul 26, 2019
@erdii erdii mentioned this pull request Jul 26, 2019
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