-
Notifications
You must be signed in to change notification settings - Fork 50
Migrate to argonaut-codecs from foreign-generic #212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
||
-- | The range of text associated with an error | ||
newtype ErrorPosition = ErrorPosition | ||
type ErrorPosition = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these types can be generically encoded and decoded via Argonaut without requiring newtypes and generic instances, so I've returned them to be raw records. However, if we really want those newtypes (for example, for more readable type errors) then I can reinstate them.
I think we can also remove the |
Good point on the |
requestBody = AXRB.String code | ||
requestBody = Just $ AXRB.Json $ encodeJson code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if the CORS issue reported in #215 is (unexpectedly) related to using AXRB.Json
instead of AXRB.String
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, great find -- it totally is. Changing this to AXRB.String $ unsafeCoerce (encodeJson code)
works as expected. I don't know why -- going to look a little deeper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, on reflection the code isn't actually JSON. It's just a string. Encoding it runs through a stringify
call, which isn't what we want; I still don't know why this would cause a CORS issue (perhaps it's a red herring?) but regardless this wasn't a correct change to make in the first place.
This PR continues #211 by migrating from
foreign-generic
toargonaut-codecs
for JSON encoding and decoding. It also updates the tests and ensures they're exercised in CI.