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.

Sign up for GitHub

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(redis): opt-in raw support #561

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

feat(redis): opt-in raw support #561

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5a70fc6
feat(redis): add `saveRawAsBinary` driver option (#559)
cjpearson Jan 3, 2025
ca6ed01
update
pi0 Jan 3, 2025
74712ec
update docs
pi0 Jan 3, 2025
fb2977a
improve normalize support
pi0 Jan 3, 2025
79d470f
lint
pi0 Jan 3, 2025
b92591e
use inherited type
pi0 Jan 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/2.drivers/redis.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ const storage = createStorage({
- `cluster`: List of redis nodes to use for cluster mode. Takes precedence over `url` and `host` options.
- `clusterOptions`: Options to use for cluster mode.
- `ttl`: Default TTL for all items in **seconds**.
- `useRaw`: If enabled, `getItemRaw` and `setItemRaw` will use binary data instead of base64 encoded strings. (this option will be enabled by default in the next major version.)
- `raw`: If enabled, `getItemRaw` and `setItemRaw` will use binary data instead of base64 encoded strings. (this option will be enabled by default in the next major version.)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would suggest something like binary/useBinary here. I think it makes the effect a bit more clear because it changes how raw data is stored, but does not enable/disable the ability to save raw data. Or it could be inverted to base64 so in the future the default value can be false.

Copy link
Member

@pi0 pi0 Jan 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I agree that in general binary could be more describing than raw. In the context of unstorage, we call binary-compatible feature raw which was the reason I was thinking to use this.

Considering it is for short-term solution, are you happy we go with raw?

(Also check fb2977a, I have extended normalization)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extending normalization to additional variants of TypedArray makes sense, but it should only allow for binary data and continue to throw when other data is passed. Otherwise it would lead to some unexpected behavior as you save an object but get a Buffer back.

That's also why I think the different naming is helpful. binary means that setItemRaw accepts binary and getItemRaw returns binary. Whereas the existing raw behavior accepts binary and other things.

Copy link
Member

@pi0 pi0 Jan 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is consistent with generic unstorage.*raw* interface behavior -- happy to change it in the future (so ALL calls to setItemRaw will be restricted to accept binary only not objects) but normally, users should use the same key either for raw or non-raw purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm would it be possible to add an additional option in that case? Either on the driver or the method? What I'm looking for is a way to save binary data and ensure it is binary and saved as binary and when getting, that I get a Buffer in return.

My assumption is that is how getItem(key, { type: 'bytes'})/setItem(key, value, { type: 'bytes'}) would work in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Yes exactly for type: 'bytes' we can get strict.

Currently, if someone passes a string to setItemRaw, getItemRaw gives a Buffer representation of Utf8 bytes which isn't that odd ,is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's what I find a bit unexpected 😅

Do you think there would be an issue if we added type: 'bytes' as a method option here? It would be just for the redis driver at the moment, but done in a forward compatible manner.

Or is it better to use something else and save that wording for later


See [ioredis](https://github.com/luin/ioredis/blob/master/API.md#new-redisport-host-options) for all available options.

Expand Down
Loading