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

Upgrade and fix windows issues #14

Closed
wants to merge 1 commit into from

Conversation

zephraph
Copy link

@zephraph zephraph commented Jun 15, 2017

WIP: DO NOT MERGE

My intent was to implement the suggestion in #13, but I couldn't even get the project to run correctly on windows. It wasn't the fault of the project itself, just weird graphql dependency issues. I started trying to solve those and... well... just went a little overboard.

All the dependencies are up-to-date. I moved off of all forked libraries. I switched over to apollo's express.js graphql implementation, being that they give the baseline of what graphql-express has, then some.

With all that being said, it's currently broken.

image

My next steps are adding tests, determining the scope and cause of the current breakages, fixing everything, and getting it into a semi-presentable state.

Any help will be appreciated.

@IvanGoncharov
Copy link
Member

IvanGoncharov commented Jun 15, 2017

@zephraph Thank you for help. I wasn't aware that Faker is broken on Windows :(
It would be great if you help us to fix it.

However we can't make Faker work with vanilla graphql-js untill graphql/graphql-js#746 is merged.Same story with express-graphql. It depends on graphql-js and it will not work with conflicting version. Switching to graphql-server-express won't help either, since you will have exactly same problem.
So dependency on forks is intentional.

Do you have issues with running Faker on Windows?
Good start would be to split your Windows related changes into separate PR.

@zephraph
Copy link
Author

I knew the forks were intentional, I just didn't know the exact issue. Thanks for the info.

I did have trouble running on Windows, yeah. It kept throwing an invariant error saying the schema must be valid and there should only be one version of graphql in node_modules. That was with the example schema that you provide when it starts. Works fine on OSX though.

The other thing was the reliance of some shell cmds that aren't consistently available on Windows. I'll pair things down a bit.

I would say there's a huge need for tests though.

@IvanGoncharov
Copy link
Member

It kept throwing an invariant error saying the schema must be valid and there should only be one version of graphql in node_modules.

Try to install faker into empty folder:

npm init
npm install graphql-faker
npm run graphql-faker
./node_modules/.bin/graphql-faker

If it helps that mean you have conflicting graphql package in global node_modules.
In any case please write about the result here.

@zephraph
Copy link
Author

Sorry, this dropped off my radar. I've been a bit busy lately. I'm going to close this PR as it's not really useful in its current state.

@zephraph zephraph closed this Sep 17, 2017
@IvanGoncharov
Copy link
Member

@zephraph No problem. You always welcome to open new PRs or issues.

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.

2 participants