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

Ignore commented code #174

Closed
jonaskello opened this issue Dec 13, 2017 · 9 comments
Closed

Ignore commented code #174

jonaskello opened this issue Dec 13, 2017 · 9 comments

Comments

@jonaskello
Copy link
Contributor

jonaskello commented Dec 13, 2017

Currently when a string with the gql tag exists but is commented, there will be an exception:

{ GraphQLError: Syntax Error ./xxxx/container-component.tsx (2:1) Cannot parse the unexpected character "/".
@dotansimha
Copy link
Owner

@jonaskello , you are right, I will fix it soon.
Also, I need to add support for gql(...) (use it as function)

@jonaskello
Copy link
Contributor Author

IIRC the checks are now done with regexp. I think maybe a better approach would be to parse it into an AST using babel or typescript. That would probably be more accurate than a regexp. But of course it would be more complex to implement and maybe slower too.

@dotansimha
Copy link
Owner

dotansimha commented Mar 13, 2018

@jonaskello Yeah sure it makes sense. Do you have a reference or example for checking a code file as AST? I'll give it a try :)
Also, it might be possible to run something like https://www.npmjs.com/package/strip-comments before running the regex...

@dotansimha
Copy link
Owner

dotansimha commented Mar 13, 2018

@jonaskello
strip-comments seems to work great (see unit tests)

Please see #192

What do you think?

@dotansimha
Copy link
Owner

Fixed in 0.8.19 :)

@jonaskello
Copy link
Contributor Author

IMO, using strip-comments is a simpler solution and therefore better :-). If there are more advanced code-analysis needed later we can look into parsing into an abstract syntax tree at that point.

@danielkcz
Copy link
Contributor

danielkcz commented Mar 14, 2018

Yea well, this wasn't a good move. See #196 ;) I think it will require an approach that @jonaskello mentioned and go with AST.

Just my two cents. In VSCode I am using https://www.npmjs.com/package/@playlyfe/gql to essentially highlight and intellisense graphql queries. That would mean it can parse it just fine and apparently works cross-language, so it might be worth investigating. Perhaps it's possible it could fulfill a duty of file watcher as well to have generated types refreshed automatically?

@jonaskello
Copy link
Contributor Author

Regarding parsing to AST I think we could either use babylon as it now has typescript support (see here). Or we could simply use typescript, see here for a simple example of traversing the AST.

@dotansimha
Copy link
Owner

@jonaskello @FredyC
We recently merged this: #907 , now the codegen will parse the code file as AST and extract the documents from it.

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

No branches or pull requests

3 participants