search

grammyjs / storages

Storage adapters for grammY sessions.
48 stars 23 forks source link

fix(firestore): add conversations support #177

Closed arthurgubaidullin closed 8 months ago

arthurgubaidullin commented 1 year ago

I fixed data serialization/deserialization to Firestore document.

The conversation plugin uses an array of arrays structure. This data type is not supported by Firestore.

After my changes, the data will be stored in binary form in the document field.

I also removed the dependency on the package, since they are not needed.

Also this will fix the issue #167.

BREAKING CHANGES: this will most likely reset all sessions.

Satont commented 1 year ago

Why would you store binary in nosql database?

I'm not familiar with firestore, insteresting what @KnorpelSenf think, sincw he's author of this adapter.

arthurgubaidullin commented 1 year ago

@Satont There are several reasons: 1) This solves the problem with the Array<Array<_>> structure in all sessions at once. 2) Sessions usually do not participate in queries and do not require indexes, which are created automatically for all fields in Firestore. For one field, it is easy to set up a ban on creating an index. 3) Binary data takes up less space and consumes less traffic. 4) The conversations plugin data can grow quite quickly due to its nature. Compact data will delay exceeding the limit.

arthurgubaidullin commented 1 year ago

@KnorpelSenf, I took the initiative to revise the adapter due to its incompatibility with the conversation plugin, as its usage causes the entire application to crash.

If you don't mind, could you please elaborate on the specific challenges you are facing? I would greatly appreciate a detailed explanation.

arthurgubaidullin commented 1 year ago

Oh I would've taken the simple path of merely adding (de)serialisation to packages/firestore/src/index.ts just like that. I was just wondering why you didn't—but then again, it's definitely a cleaner abstraction to add more interfaces etc.

I prefer more explicit code, less room for error.

I assume that you use this adapter actively (in contrast to me)?

Right now I'm using my own version of the adapter.

Would you be available in the future if we need to change something?

I don't know.

KnorpelSenf commented 1 year ago

I'll leave it up to @Satont if he wants to deal with the complexity you introduce. The motivation behind this pull request is reasonable.

Satont commented 8 months ago

@arthurgubaidullin rebase please

arthurgubaidullin commented 8 months ago

@Satont Something has gone wrong. I don't want to fix it. I do not have time. You can delete PR.