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

split('/n') does not work on Windows #61

Closed
yurigenin opened this issue Mar 14, 2017 · 7 comments · Fixed by #89
Closed

split('/n') does not work on Windows #61

yurigenin opened this issue Mar 14, 2017 · 7 comments · Fixed by #89

Comments

@yurigenin
Copy link

yurigenin commented Mar 14, 2017

The loader fails to process fragments correctly on Windows because split('\n') does not work there. Modifying it to use split('\n\r') works, however. So, you need to determine the correct newline/carriagereturn characters in a platform independent way.

@jnwng
Copy link
Contributor

jnwng commented Mar 14, 2017

@yurigenin ah, yes, that's correct — good catch!

happy to accept a PR to fix this (maybe with a test if its something we can capture in a test), and we can get this into 1.3.2 as well as 2.0

@jbaxleyiii
Copy link

@jnwng I'll take care of this this week!

@wajb
Copy link

wajb commented Jun 5, 2017

Hi. This has broken cross platform support for us. Since graphql files can be written on one platform should not mean they can't be read from another.

The specifications state

line terminators...have no significance to the semantic meaning of a GraphQL query document.

Therefore, I believe, a Windows platform ought to be able to load a file written in Linux etc.

Can we split by /(\r?\n)+/ in the loader.js?

@jnwng
Copy link
Contributor

jnwng commented Jun 5, 2017 via email

@justindoherty
Copy link

Unfortunately not, the query documents were written to have \n as the line terminator then when we try and run it on a Windows machine the os.EOL returns \r\n so it doesn't end up splitting on anything.

@jnwng
Copy link
Contributor

jnwng commented Jun 5, 2017 via email

@wajb
Copy link

wajb commented Jun 20, 2017

I see you fixed it already but I've submitted a PR with test coverage. I have also submitted a contributor agreement by email. Thanks.

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

Successfully merging a pull request may close this issue.

5 participants