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

Feature/custom directives #152

Merged
merged 8 commits into from
Feb 14, 2018

Conversation

giautm
Copy link
Contributor

@giautm giautm commented Feb 1, 2018

No description provided.

@fabien0102
Copy link
Contributor

Nice! A little snipet to illustrate how it’s works maybe? :)

@fabien0102
Copy link
Contributor

I guess I have my answer here ^^ ardatan/graphql-tools#518 I will try this today :)

@schickling
Copy link
Contributor

schickling commented Feb 1, 2018

Thanks a lot! This looks great @giautm!

As @fabien0102 mentioned, would be great if you could also add an example using this feature in this PR.

@giautm
Copy link
Contributor Author

giautm commented Feb 1, 2018

@schickling : I think we should have some unit-test, I'm a little worried when adding a new feature.
😢

@schickling
Copy link
Contributor

I completely agree, @giautm. Would be great if you could add some testing setup. Either unit or integration tests.

@marktani marktani removed the feature label Feb 3, 2018
@fabien0102
Copy link
Contributor

Example added! It's works perfectly, nice job @giautm 😉

For the integration tests, we can use directly the examples projects with supertest, and just execute the README.md queries. It can be a good start and avoid to duplicate code. The good part is that each integration test must have a workable example associate (but it can also be a bad part ^^).

If this solution can fit with the project, I can begin to work on a POC and make a PR to open the discussion 😉

Add a working example for custom directives
@schickling
Copy link
Contributor

This looks really great! I've merged this already since the changes seem very straightforward.

@fabien0102 @giautm can you create a new PR for the testing setup? I'd recommend using ava or jest as a test runner/framework.

@schickling schickling merged commit 7720d71 into dotansimha:master Feb 14, 2018
@fabien0102
Copy link
Contributor

Always jest for me 😉 I write this in my personal todolist ^ ^

@fabien0102
Copy link
Contributor

fabien0102 commented Feb 14, 2018

@schickling Please note that the example may not work due to the package version of graphql-yoga inside the example (https://github.com/graphcool/graphql-yoga/blob/master/examples/custom-directives/package.json)

I don't know yet how you deals with this, so carreful (I tried with yarn link locally)

(it can be a good idea to put lastest as graphql-yoga version to every examples 😉)

@sflahave
Copy link
Contributor

I just tried the custom-directives example and it doesn't seem to work. The directive resolvers are never invoked.

@schickling
Copy link
Contributor

@sflahave would you mind creating a new issue for this?

@sflahave
Copy link
Contributor

Sure - actually it works after updating to v1.6.0. The example's package.json specifies v1.2.0 currently.

@schickling
Copy link
Contributor

Thanks a lot @sflahave. Can you create a PR updating the example version then?

@sflahave
Copy link
Contributor

sflahave commented Mar 17, 2018

@schickling - sure, I'd be happy to do that.
#215

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.

5 participants