Skip to content

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