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 transformer option #50

Merged
merged 6 commits into from Feb 14, 2023
Merged

Add transformer option #50

merged 6 commits into from Feb 14, 2023

Conversation

hmbrg
Copy link
Contributor

@hmbrg hmbrg commented Feb 7, 2023

This implements #46.

This allows passing a transformer library like superjson to serialize data types that are not supported by JSON.stringify.
The API for this option was inspired by https://trpc.io/docs/data-transformers#using-superjson.

You can use a transformer like this on a per-cache basis:

import superjson from "superjson"

const cache = new Cache({
  storage: createStorage(),
  transformer: superjson
})

Alternatively, a transformer can be added for a specific function:

import superjson from "superjson"

cache.define('fetchSomething', {
  transformer: superjson
}, async (query, cacheKey) => {
  return { k: query }
})

I've tried creating tests for this, but the test are failing without any error message.
Since I'm not too familiar with tap for testing, I was hoping somebody could look into this.

I hope this is helpful!

Copy link
Owner

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the test is good but there was a bug in your implementation.

src/cache.js Outdated
@@ -334,10 +341,16 @@ class Wrapper {
}

async get (key) {
if (this.transformer) {
return this.transformer.deserialize(this.storage.get(key))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return this.transformer.deserialize(this.storage.get(key))
return this.transformer.deserialize(await this.storage.get(key))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks! I changed this, but sadly the error remains. I've also added documentation and an example to the README.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I've managed to fix all the errors. There actually was another bug in the implementation.

Copy link
Owner

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina
Copy link
Owner

@hmbrg
Copy link
Contributor Author

hmbrg commented Feb 13, 2023

Apparently the code coverage is too low: https://github.com/mcollina/async-cache-dedupe/actions/runs/4162364135/jobs/7203097232#step:6:1656

Fixed it!

Copy link
Owner

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina merged commit a2d2236 into mcollina:main Feb 14, 2023
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.

None yet

2 participants