Skip to content

Feat/cache package#80

Open
anti-duhring wants to merge 8 commits into
mainfrom
feat/cache_package
Open

Feat/cache package#80
anti-duhring wants to merge 8 commits into
mainfrom
feat/cache_package

Conversation

@anti-duhring

Copy link
Copy Markdown
Collaborator
  • Setup cache-only example
  • Setup cache package

@changeset-bot

changeset-bot Bot commented Oct 31, 2023

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 73b0f19

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@nicolasmelo1 nicolasmelo1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remind to do a git pull, there is lots of code that i have already changed. So it's bringing up 149 files changes hahahahahhaha.

Do a git pull origin main to get the changes from the main. Besides that i'll comment out the changes on the codebase.

@anti-duhring

Copy link
Copy Markdown
Collaborator Author

Remind to do a git pull, there is lots of code that i have already changed. So it's bringing up 149 files changes hahahahahhaha.

Do a git pull origin main to get the changes from the main. Besides that i'll comment out the changes on the codebase.

Noted! I think we should try keep the main branch up to date to avoid merge conflicts. On this feature I'm gonna modify only the libs, packages and the cache-example dir, nothing that would bring merge conflicts on what you're doing

@nicolasmelo1 nicolasmelo1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Everything is great, just don't forget to do a git pull origin main. I already applied some changes but it's appearing how it was before for them. (It's appearing 149 changes hahahahahahaha)

Besides that LGTM 👌

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remember that i said to return a function from new on CacheAdapter?

Probably you will need some modifications here. You will need to cache the callback and the adapter instance.

Think with me here

const cacheAdapterInitializer = RedisCacheAdapter.new({});

and then

cacheAdapterInitializer() to get the actual instance.

So probably you need to store this callback here. And then it's up to you. The CacheAdapter instance you can store either here, or in the Cache (on src/cache.ts right above this file) instance directly.

This will need some modifications from your Cache class as well

@anti-duhring anti-duhring Nov 8, 2023

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Remember that i said to return a function from new on CacheAdapter?

Probably you will need some modifications here. You will need to cache the callback and the adapter instance.

Think with me here

const cacheAdapterInitializer = RedisCacheAdapter.new({});

and then

cacheAdapterInitializer() to get the actual instance.

So probably you need to store this callback here. And then it's up to you. The CacheAdapter instance you can store either here, or in the Cache (on src/cache.ts right above this file) instance directly.

This will need some modifications from your Cache class as well

I think I miss the point here. What changes should I do on the Cache class? Do you mean on the #getCachedAdapter method specifically? I thought the callback that will be stored on cacheAdapterInitializer will be called on the load or ready step of the lifecycle.

Comment thread packages/cache/src/cache.ts
Comment thread packages/cache/src/cache.ts
Comment thread packages/cache/src/adapter.ts
Comment thread packages/cache/src/domain.ts
Comment thread packages/cache/src/index.ts
})
}

async set(_key: string, _value: any) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

add the option to have a TTL defined when setting a value

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The options should been passed as parameter on the set method or should be passed on the "new" method? What pattern palmares use for this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is on the example folder. Create a cache adapter on the redis folder, What do you think about already creating an adapter for Redis?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think it's a great idea. Where should I put this? Inside the libs folder?

return cachedAdapter?.get(key);
}

async set(key: string, value: any) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Support TTL. And don't forget that caching by default is only strings, so you need to do a JSON.stringify and then a JSON.parse when retrieving the data.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we can make TS better on this.

Cache could receive a generic like

Cache {}

Then the user could define the types for the data it store, we can also use it on set

cache.set('key', 'my-value')

We will write set as

async set<TKey extends string, TValue extends any>(key: TKey, value: TValue) {
return this as Promise<Cache<TData & { [Key in TKey]: TValue }>>
}

So pretty much whenever you do

cache.set()

we

Will return cache but typed for the user.

cache = await cache.set('value', 1)

cache.get('value') // Promise

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For better Typing we can even go further, remove the async on set function. With that we will need to be creative.

cache = cache.set('value', 1)

This will NOT remove the async on the adapter, the adapter will STILL be async.

Something like.

class Cache<TData extends object> {
   #setPromisesByKey = {};
   
   async get<TKey extends string>(key: TKey) {
       if (this.#setPromisesByKey[key]) await this.#setPromisesByKey[key];
       
       // get the value from the adapter
   }
   
   set<TKey extends string, TValue extends any>(key: TKey, value: TValue) {
       /// set from adapter
       
       // I think you can make this better but you got the idea.
       const promiseToWait = this.#getCachedAdapter().then((cachedAdapter) => {
             if (cachedAdapter) cachedAdapter.set(key, value).then(() => {
                delete this.#setPromisesByKey[key]
             })
        })
        
       this.#setPromisesByKey[key] = promiseToWait;
       
       return this as Promise<Cache<TData & { [Key in TKey]: TValue }>>
    }
}

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