Satont / grammy-typeorm-storage

MIT License
1 stars 1 forks source link

Do not transfer session data twice on write #1

Closed KnorpelSenf closed 3 years ago

KnorpelSenf commented 3 years ago

https://github.com/Satont/grammy-typeorm-storage/blob/e4aaaef59fd0da66f96827d6e4a1d998c996a69e/src/index.ts#L23 looks like the session data is read and transferred entirely. All that is needed would be to find out if the specified key exists.

As it seems like EXISTS isn't implement in TypeORM yet (confer https://github.com/typeorm/typeorm/issues/2815) I suggest to specify only the key column to be returned from find.

Satont commented 3 years ago

True.

I can add some select, but i'm not sure does it will work with typeorm + mongodb. So i choiced that way to check does record is exists.

Satont commented 3 years ago

I'll write some additional tests for mongo i guess.

Satont commented 3 years ago

Just tested mongo, seems like it returns all keys instead of partical key. But it should work with SQL db's. Anyways tests is passing, but i'm not tested it on actual mongodb. Can be bugged thought.

So, i created new release and published it on npmjs. Also did PR for grammy website (https://github.com/grammyjs/website/pull/18)

If you wan't to discuss something, i'll keep that issue open.

Satont commented 3 years ago

If there is way to convert nodejs lib to deno i'll glad to make it, by the way. But i'm not using deno personaly.

KnorpelSenf commented 3 years ago

Just tested mongo, seems like it returns all keys instead of partical key.

It's completely fine because it's a performance optimisation that we're talking about, and not an actual bug. I suggest to leave it as-is, unless someone discovers a problem. If a more efficient storage adapter is required for someone, then a native mongo adapter could be created.

If there is way to convert nodejs lib to deno

Given that all dependencies of a project have equivalents on both platforms, it's easier to develop for Deno, and create a Node.js build from it, e.g. using deno2node. However, TypeORM does not exist for Deno. It therefore makes more sense to develop a second, independent storage adapter that uses denodb or another ORM for Deno.

Satont commented 3 years ago

Given that all dependencies of a project have equivalents on both platforms, it's easier to develop for Deno, and create a Node.js build from it, e.g. using deno2node. However, TypeORM does not exist for Deno. It therefore makes more sense to develop a second, independent storage adapter that uses denodb or another ORM for Deno.

True, i totally forgot there is no TypeORM for deno, sorry.

So, i think everything just fine, so i close this issue. Feel free to create new one or keep talking in that, if you have some minds.

KnorpelSenf commented 3 years ago

Sounds good! Thanks again for your work :)