Allow for non abstract cache options more easily

[tbouasli]

Prerequisites

• [X] I have written a descriptive issue title
• [X] I have searched existing issues to ensure the issue has not already been raised

Issue

This library is highly coupled to abtract cache, it would be nice to not have to depend on that library since it is not being actively maintained for about 3 years and is using an old version of ioredis.

There is no need to break compatibility with it, only to allow for other options. The types seem to be inspired by the package abstract-cache-redis and it is troublesome to reimplement the interface for it on every project, or to depend on the package to be updated.

I'm willing to implement it.

[jsumners]

it would be nice to not have to depend on that library since it is not being actively maintained for about 3 years

I haven't seen any PRs submitted to change anything about the module. It's possible that the modules are simply stable and don't need "active maintenance."

[tbouasli]

@jsumners that's fair, but an old version of ioredis makes the interface hard to implement on the same way with the new version. If you are using abstract-cache, I don't see any problem with that, it's already implemented, but to reimplement that interface with the new version is cumbersome.

It's not that bad, but it is enough for me to justify not using fastify-caching, and I wish it weren't so. If I'm the only one that feels that way I'm fine creating my own library, but I alway prefer to check if anyone else agrees with me so that we can implement it on the core codebase and more people can benefit from it.

[Eomm]

and it is troublesome to reimplement the interface for it on every project

I'm not a TS developer and I don't understand this sentence - could you help me with an example?

[tbouasli]

Of course, I'll implement the custom adapter and show why I belive it is more trouble than it is worth!

[tbouasli]

@Eomm I won't implement the whole adapter because it would take too much time for a sunday, but here is a snippet of what would have to be done. I have not tested it, it's is just an WIP of what I would need to develop in order to not use abstract-cache.

import type { AbstractCacheCompliantObject } from "@fastify/caching";
import { redis } from "./client";

function mapKey(
  inputKey: string | { id: string; segment: string },
  segment: string
): string {
  const parts: string[] = [];
  if (typeof inputKey === "string") {
    parts.push(encodeURIComponent(segment));
    parts.push(encodeURIComponent(inputKey));
  } else {
    parts.push(encodeURIComponent(inputKey.segment));
    parts.push(encodeURIComponent(inputKey.id));
  }
  return parts.join(":");
}

export class RedisAdapter implements AbstractCacheCompliantObject {
  private config: {
    segment: string;
  };

  set(
    key: string | { id: string; segment: string },
    value: unknown,
    timeToLive: number,
    callback?: (error: unknown, result: unknown) => void
  ): void {
    redis.set(
      mapKey(key, this.config.segment),
      JSON.stringify({ item: value, stored: new Date(), ttl: timeToLive }),
      callback
    );
  }

  async get<T>(
    key: string | { id: string; segment: string },
    callback?: (error: unknown, result: any) => void
  ): Promise<any | null> {
    const mappedKey = mapKey(key, this.config.segment);

    const result = await redis.pipeline().ttl(mappedKey).get(mappedKey).exec();

    if (!result) {
      return null;
    }

    const [ttl, value] = result;

    if (!value) {
      return null;
    }

    const parsedValue = JSON.parse(value[1] as string);

    const expires = ttl[1] + parsedValue.stored;

    const newTTL = expires - Date.now();

    return {
      item: parsedValue.item,
      stored: parsedValue.stored,
      ttl: newTTL,
    };
  }
}

The typings are probably wrong, as I said more trouble than it is worth, at this point I would just go on to write my own caching layer.

[jsumners]

I don't know what any of that is. What I can tell you is that this module was written to provide an easy to use server-side cache. It was designed around abstract-cache in order to provide a contract between the module and the module(s) used to provide the data store(s). The abstract-cache module provides nothing more than a well defined API for other modules to implement. The abstract-cache-redis module is one such implementor. If you think that module needs updates, feel free to submit PRs to get it there.

[tbouasli]

That's my point though, the only way I can reasonably use this project is by adopting abstract-cache and it's interface, as you said that decision was made by design, and that's fine. Although I do not believe a core fastify package should be so dependent on an package that provides nothing more than an interface that could easily be internalized to make it easier to contribute to.

I'm new here, I wont push this matter any further. Thanks for explaining.

[Uzlopak]

I would like to discuss this further.

abstract-cache is a package by @jsumners. The same with abstract-cache-redis. Why dont you provide a PR for abstract-cache-redis to upgrade the redis version? I am pretty sure, that @jsumners would consider it positively. Than you wont need to reinvent the wheel.

[tbouasli]

I'm aware of that and I'm sure he would. the issue is more about the design of the solution than the solution itself.

Adopting abstract-cache and creating modules that implement it or contributing to abstract-cache-redis would 100% fix the problem, and I will do so. That's why I closed the issue, i disagree with the design but it works and was a conscious decision.

I will implement the newer version of ioredis on abstract-cache-redis and that will solve the issue.

[tbouasli]

@Uzlopak We could continue the discussion on the matter of the design decisions if that's open for debate, which is what I am more concerned about. Since I am new to OSS and don't want seem hostile, I don't want to push changes that are not welcome.

I'll explain my concern a little further.

I belive that making the core interface for implementing cache drivers a depenency will make most of the development effort occour outside of this package, and make integrating changes more challanging. This is a pretty simple project and I don't see the benefit in that. That's all. Not a huge problem, I just don't get why that decison was made.

Again, I don't like to be the guy that drops in a project and wants to change everything, so if that is unreasonable, that's ok. Although I do not see why that would be the case.

[jsumners]

The point is any module can be used that conforms to the requirements. It makes things easier for people to bring their own stores.

[tbouasli]

@jsumners everyone seems to feel the same way, so I'm going to assume I am wrong. I'll implement the new version of redis on abstract-cache as I mentioned and will see if that see what I am missing. If anyone want's to join the discussion I am happy to reply, but from my side we can close this issue.