graphile / crystal-pre-merge

Repository moved
https://github.com/graphile/crystal
39 stars 8 forks source link

Experimental ws for H3Grafserv #495

Closed Dodobibi closed 1 year ago

Dodobibi commented 1 year ago

This PR add :

benjie commented 1 year ago

Please rebase on the latest main branch.

Could you explain why we need two adaptors - is there a divergence in the APIs these two adaptors need to call, or is it to just make the new methods clearly experimental? If the latter, please integrate into the core adaptor and just use experimental in the method names.

Dodobibi commented 1 year ago

Yes, It was just for making the new methods clearly experimental. New commit rebased.

benjie commented 1 year ago

Nice! From a quick scan this is looking really good (will do a better review later). Have you tested it well and are happy it works? Could you add a page to the documentation for it; it would be a h3.md file created here:

https://github.com/benjie/crystal/tree/main/grafast/website/grafserv/servers

Ideally you should document how to use it with a nuxt app ❤️

Dodobibi commented 1 year ago

Have you tested it well and are happy it works?

Yes, for h3 as a standalone server and for nuxt too.

Could you add a page to the documentation for it

Yes, I will try to draft this, but you will have to (or @jemgillam) review my English syntax...

Ideally you should document how to use it with a nuxt app

Of course.

benjie commented 1 year ago

We tend to always edit the documentation whether the author is a native English speaker or not, so don't worry about that! Mostly I care about the instructions themselves, since I don't currently use Nuxt.

peteromano commented 1 year ago

Hi guys! I know you're busy this week benjie, any chance of merging this this week?

If not, I'll pull the branch and work off/test that for now.

@Dodobibi fyi I'm also working on nuxt 3 integration. Can collab of the discord channel @peteromano 🙂

I can help clean up the documentation when I'm able to test/use this pr. I think we can get nuxt working with less files in your Readme example (maybe just need one plugin, might not need the module setup etc.)

benjie commented 1 year ago

Sorry it's probably going to be closer to October before I get around to properly testing this and merging it. Are you able to run it as an external adaptor? You should be - there should be no need for this to be in core for it to work; and if there is issues I want to know about them so I can expose the relevant things so you can use it.

peteromano commented 1 year ago

Yea it seemed to work yesterday, gonna spend some more time with it later this week, no rush.