search

hasura / pg-client-hs

A low level Haskell library to connect to postgres
Apache License 2.0
25 stars 15 forks source link

Tries to fix 'Lift' instance propagation for 'Query' #36

Closed jkachmar closed 3 years ago

jkachmar commented 3 years ago

First, some context from 44da7ba's message:

The 'Lift' instance propagation for 'Text interacts strangely between
'pg-client-hs' and 'graphql-engine'.

This project compiles successfully (locally) without this change,
however the 'Lift' instance for 'Text' fails to propagate when it's used
within 'graphql-engine'.

Rather than trying to juggle the orphan instance exported by
'th-lift-instances', it seems easier to just do the same thing that it
does (i.e. unpack 'Text' to 'String' before lifting).

Honestly I'm pretty confused about what's going on here; when I compiled 0fae3bf9daad198e774ac6483c8ba552a4d19fc5 locally and tested it in my REPL everything seemed to work, but when I compiled it as a dependency of graphql-engine it started failing with an error stating that there was no Lift instance for Text.

If I were to guess, I'd say that somehow th-lift-instances was being brought into scope in my REPL session but was not propagated when the modules were brought into scope within graphql-engine. It actually seems like th-lift and th-lift-instances aren't being used within this project at all?

So there seem to be two ways to handle this:

  1. Avoid the th-lift-instances orphans entirely by converting Text -> String and using fromString :: String -> Query to construct the Query in the splice
    • This is what I've implemented in 44da7ba
  2. Properly pull in Instances.TH.Lift within Database.PG.Query.Transaction and ensure that the Lift instance for Text is propagated such that graphql-engine picks it up
    • In this case I'm not too worried about orphan instance weirdness since we control the main project which consumes this library
    • That being said, packdeps seems to indicate that this library isn't actually as widely used as I would have thought
paf31 commented 3 years ago

Looks good to me, but yes, let's add that comment.

@0x777 might like to review this also, but hopefully this should unblock your local setup, yes?

jkachmar commented 3 years ago

@0x777 If you feel like reviewing this, you might also want to take a look at #35 as well for additional context.

@paf31 My local setup should actually be fine as-is, this was just a bit of a nerd-snipe for me that I thought was going to be quicker until I started running into these issues...