search

electric-sql / pglite

Lightweight WASM Postgres with real-time, reactive bindings.
https://pglite.dev
Apache License 2.0
9.49k stars 204 forks source link

Fix array serialization #306

Closed alexgleason closed 2 months ago

alexgleason commented 2 months ago

Fixes https://github.com/electric-sql/pglite/issues/220

alexgleason commented 2 months ago

@samwillis Is there a better way to run tests than this? This way is slow and quite painful:

pnpm build && pnpm exec vitest run -t 'basic esm array params' tests/basic.test.js
alexgleason commented 2 months ago

I cleaned it up and I think it's a bit better now. Let me know if you have any suggestions!

samwillis commented 2 months ago

Thanks for picking this up @alexgleason!

It's the end of the day for me here and so I'll take a proper look in the morning. I don't suppose you could just jot down the strategy you've gone with, from a quick glance (on my phone cooking dinner) I didn't quite follow what fixedParams was doing. Is this influenced by any other JS Postgres's clients?

alexgleason commented 2 months ago

@samwillis After sending Parse, we now send a Describe (S type) to get parameter info from Postgres.

I loop through parsedParams, and if the oid we inferred with JS code doesn't match the oid Postgres gave us, I re-parse it with the new oid. Then fixedParams gets passed down instead of parsedParams.

EDIT: It's based on what Postgres.js does, but these two libraries are very different so I had to write this code from scratch.

EDIT2: I removed fixedParams and reworked that code to not parse the params twice.

alexgleason commented 2 months ago

I published this branch to https://www.npmjs.com/package/@soapbox.pub/pglite, pulled it into my tests, and would you look at that:

Screenshot from 2024-09-10 15-45-53

Finally able to use pglite in my application.

samwillis commented 2 months ago

Hey @alexgleason

This is great, I really like the strategy of asking PG what the types of the params should be.

I want to spend some time refactoring the type support, and will use this as the base. Plan for early next week.

alexgleason commented 2 months ago

I updated this to remove the unnecessary JS type inference code I added. So it's simpler now, and still works perfectly.

I think this is probably acceptable to merge, as there is not much baggage. I would say optionally the function serializeType in types.ts could be removed, but it's still used by serializeArray, which I think is fine.

@samwillis if you have any specific gripes feel free to give me a scathing review like I'm you're employee. I have time to fix it, and match your preferred code style etc. Since PGlite is also required for my goals (which is my full time job). I just need to get it in a place where everything works right in my project, then it's done. I also need your help with https://github.com/electric-sql/pglite/issues/327 but I think it makes sense to do this one first.

samwillis commented 2 months ago

Hey @alexgleason

This had made it to the top of my list. I'm refactoring the type support based on this, and so should fix the JSON/Array stuff along with some bugs in the live queries.

This PR failed a few test, but I'm making quite a few changes. I'll ping you on the new PR I make (based on a fork of this) in a couple days.

alexgleason commented 2 months ago

Thank you very much @samwillis !

samwillis commented 2 months ago

Closing in favour of #340 which is based on this but with a larger refactor.