Skip to content
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

Add JSONResponse type #1494

Closed
wants to merge 1 commit into from
Closed

Add JSONResponse type #1494

wants to merge 1 commit into from

Conversation

MaxGabriel
Copy link
Member

Closes #1481

Before submitting your PR, check that you've:

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

-- > return $ JSONResponse $ CreateUserResponse userId
--
-- @since 1.6.3
data JSONResponse a = ToJSON a => JSONResponse a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better without the datatype context ToJSON a => here. I don't remember all of the arguments, but it's typically considered a bad extension.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, this gives some reasons https://wiki.haskell.org/Data_declaration_with_constraint

Haven’t tried this, but the wiki recommends replacing with a GADT

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think a GADT is necessary here. I'd replace with just:

newtype JSONResponse a = JSONResponse a

Is anything lost in such a change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not that bad, but the type error is worse if you move the ToJSON constraint to ToContent and ToTypedContent:

/Users/maximiliantagher/Documents/Mercury/hs/mercury-web-backend/src/Application.hs:67:1: error:
    • No instance for (ToJSON Foo) arising from a use of ‘yesodRunner’
    • In the expression:
        yesodRunner getHomeR env6761_a2uNs (Just HomeR) req6761_a2uNt
      In a case alternative:
          "GET"
            -> yesodRunner getHomeR env6761_a2uNs (Just HomeR) req6761_a2uNt
      In the expression:
        case Network.Wai.Internal.requestMethod req6761_a2uNt of {
          "GET"
            -> yesodRunner getHomeR env6761_a2uNs (Just HomeR) req6761_a2uNt
          _ -> yesodRunner
                 (void badMethod) env6761_a2uNs (Just HomeR) req6761_a2uNt }

vs if the constraint is on JSONResponse itself:

/Users/maximiliantagher/Documents/Mercury/hs/mercury-web-backend/src/Handler/Home.hs:16:21: error:
    • No instance for (ToJSON Foo) arising from a use of ‘JSONResponse’
    • In the second argument of ‘($)’, namely ‘JSONResponse [Foo]’
      In the expression: return $ JSONResponse [Foo]
      In an equation for ‘getHomeR’:
          getHomeR = return $ JSONResponse [Foo]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the GADT approach solve that?

@StevenXL
Copy link
Member

The version with GADTs works pretty well. The image below shows the declaration of the JSONResponse type constructor using GADTs, the type signature of the JSONResponse data constructor, and the error when you try apply JSONResponse to a value that is not an instance of ToJSON. I think it works out very well.

@MaxGabriel Given that it has been a long time since your PR, I'll make another PR with GADTs and reference this as well as #1481. (I can always back off if you say so. Thank you).

Screen Shot 2019-04-12 at 3 18 54 PM

@StevenXL StevenXL mentioned this pull request Apr 12, 2019
5 tasks
@StevenXL
Copy link
Member

Closed by #1592

@StevenXL StevenXL closed this Apr 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSON return type for Handlers?
3 participants