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

Spec compliant error #33

Open
lxcid opened this issue Mar 27, 2018 · 19 comments
Open

Spec compliant error #33

lxcid opened this issue Mar 27, 2018 · 19 comments
Assignees
Milestone

Comments

@lxcid
Copy link
Contributor

lxcid commented Mar 27, 2018

Recently, GraphQL Response Specification got updated with a suggestion to place custom data in error in the extensions object. The spec can be found here https://github.com/facebook/graphql/blob/master/spec/Section%207%20--%20Response.md

An excerpt of the relevant section:

GraphQL services may provide an additional entry to errors with key extensions. This entry, if set, must have a map as its value. This entry is reserved for implementors to add additional information to errors however they see fit, and there are no additional restrictions on its contents.

{
  "errors": [
    {
      "message": "Name for character with ID 1002 could not be fetched.",
      "locations": [ { "line": 6, "column": 7 } ],
      "path": [ "hero", "heroFriends", 1, "name" ],
      "extensions": {
        "code": "CAN_NOT_FETCH_BY_ID",
        "timestamp": "Fri Feb 9 14:33:09 UTC 2018"
      }
    }
  ]
}
@thebigredgeek
Copy link
Owner

thebigredgeek commented Mar 28, 2018

PRs are welcome! I'd don't really have time to make this update at the moment

@lxcid
Copy link
Contributor Author

lxcid commented Mar 28, 2018

Sure man PR at #35

@thebigredgeek
Copy link
Owner

Thanks! Merged! If you wanna have a whack at fixing 1.7.1 bugs and add more test coverage, I am also happy to merge more PRs. I can't release this (2.0.0, since it's a breaking change) until we get the bugs from the TS refactor sorted out. Also, I am looking for additional core contributors if you are interested

@lxcid
Copy link
Contributor Author

lxcid commented Mar 28, 2018

Thanks! I sent a PR for the constructor fix in #34.

The fix is very similar to #29. But one difference is that newConfig or (arguments[2]) have the highest precedent when merging. It also make sure to merge data and options.

I can rebase on top of the reverted on top of the reverted PR and continue the discussion from there.

@joshmanders
Copy link

@thebigredgeek I'm using this heavily in a few projects and plan on releasing an open source boilerplate that includes it, I'm more than willing to help maintain it.

@lxcid
Copy link
Contributor Author

lxcid commented Apr 1, 2018

I can help in an ad hoc and second opinion manner.

@thebigredgeek
Copy link
Owner

Can we create a second PR for this? I'd like to release in 2.0

@thebigredgeek
Copy link
Owner

Bump

@lxcid
Copy link
Contributor Author

lxcid commented Apr 18, 2018

Sorry I miss this message. I'll try send a PR your way today or tomorrow!

@theGlenn
Copy link

theGlenn commented Jun 8, 2018

@lxcid @thebigredgeek bump

@theGlenn
Copy link

Why has #35 been reverted 🤔

@theGlenn theGlenn added this to the Release V2 milestone Jul 21, 2018
@theGlenn
Copy link

theGlenn commented Jul 21, 2018

@lxcid Could you make another PR against version-2 ?

@thebigredgeek
Copy link
Owner

This should be against version 2 should it not? I think that was why I reverted originally, too much push back about the breaking change.

@lxcid
Copy link
Contributor Author

lxcid commented Jul 25, 2018

sorry for the afk, i try to work on it now…

@aneilbaboo
Copy link

Hi, @thebigredgeek I notice that lxcid's PR is in the version-2 branch, but npm is still at 1.9.0.

Is it ok to use version-2, or are there still some remaining issues?

@thebigredgeek
Copy link
Owner

@lxcid was this ever completed and merged?

@kylewhitaker
Copy link

@thebigredgeek How can I help resolve this issue and get V2 published? Right now we're monkey-patching the error response object at the last second as a workaround. Should we switch over to a different package that is better supported? Thanks for any help. cc: @jonesmac @tarazena @mwlevel20

@darrylyoung
Copy link

Is there anything anybody can do to move this forwards and get 2.0 released?

@thebigredgeek
Copy link
Owner

@kylewhitaker you can put together a PR that implements the desired functionality to resume spec compliance, against version 2. Once that is done I'll release it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants