-
-
Notifications
You must be signed in to change notification settings - Fork 87
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
Feat various cleanup #65
base: master
Are you sure you want to change the base?
Conversation
- this offers better flow for writing tests and nested describes changing the test setup.
This is derived from PR f#55
@@ -191,8 +203,7 @@ | |||
* {a: {b: {c: 1, d: 2}}}, "a.b.c" => 1 | |||
*/ | |||
GraphQLClient.prototype.fragmentPath = function (fragments, path) { | |||
var getter = new Function("fragments", "return fragments." + path.replace(/\./g, FRAGMENT_SEPERATOR)) | |||
var obj = getter(fragments) | |||
var obj = fragments[path.replace(/\./g, FRAGMENT_SEPERATOR)] |
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.
why this is changed?
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.
Because it's equivalent, safer, and mentioned in #49
@f I removed the "content-length" commit from this PR, as apparently that is a forbidden header by the XMLHttpRequest, so there is no point in manually setting it, the browser should be doing that work for us. |
cd8f63a
to
31f6fac
Compare
This performs a few small pieces of cleanup and fixes.
This PR include the changes from
#22
#49
#55
And obviates #64 (update of packages no longer includes this minimist)
Also now that there is jest test rig this PR ensures tests were added for all the above (or tests passing in the case of #49)