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

Update node, redis lib; deprecate hydration lib #6

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

Conversation

Qulinary
Copy link
Collaborator

Hey! Awesome lib. We made a few tweaks to get this working on more modern versions of node. Build is happily passing:

https://travis-ci.org/QulinaryOrg/node-stow

@cpsubrian
Copy link
Owner

Hey, thanks for the PR! Glad to know someone else is getting some good use out of stow, it's been successfully humming along for us for years.

The one thing blocking me from immediately merging this is the deprecation on hydration. This would be a huge breaking change for anyone (possibly me) relying on proper serialization of Date and RegExp objects. At best this would be a divergence from the current API and a Major version update, at worst a deal-breaker.

I wonder if there is a more up-to-date serialization approach that supports native JS types or if we can get hydration updated so we can still use it.

@carlos8f

@cpsubrian
Copy link
Owner

Also probably worth removing the engines field altogether and making sure it runs on node 4.x also (sinces that's the new stable build).

@cpsubrian
Copy link
Owner

Note: I'm fine with the switch to plain redis, since this already supports client which could be an haredis client anyhow.

@matmar10
Copy link
Collaborator

Great, thanks for quick comments, @cpsubrian Do you have a test case (or high-level description ... I can write the test) of what is needed regarding serialization support? The changes didn't fail any existing test cases, hence didn't think it was a breaking change.

@matmar10
Copy link
Collaborator

This looks promising: https://github.com/skeeto/resurrect-js

@cpsubrian
Copy link
Owner

Yeah this is a case where there were implied requirements not in the tests, my bad! I think we'd want to add a test with a cached object that has Date and RegExp objects in them and make sure that when they are retrieved from the cache they are re-instantiated properly. Of note is the fact that hydration is recursive, so a Date 4 levels deep in an object will still be properly handled.

Agreed that resurrect looks promising.

@cpsubrian
Copy link
Owner

Also, I'm giving you commit access here, but I'd still like to discuss changes in the form of PRs.

@gustawdaniel
Copy link

Why this pull request was abandoned?

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.

4 participants