type lookup in hint fails

[timsuchanek]

Hi,

we're using the codemirror-graphql in the GraphQL Playground and experience problems, that the type property of a suggestion is undefined.
It happens in this line of code:
https://github.com/graphql/codemirror-graphql/blob/master/src/hint.js#L66

The problem is, that item.detail can contain ! in the case of a non-nullable type.
item.detail comes from here https://github.com/graphql/graphql-language-service/blob/master/packages/interface/src/getAutocompleteSuggestions.js#L97

schema.getType() however is just returning getTypeMap()[name] - as there is no type with the name User!, undefined is returned.
https://github.com/graphql/graphql-js/blob/master/src/type/schema.js#L182

Now is the question what the solution for this could be.
The quick-fix, that would work for us, is just not calling .getType() in hint anymore.
But I guess that the type lookup has been added for a reason.

@asiandrummer would be awesome to hear your take on that.

Thanks!

[asiandrummer]

@timsuchanek sorry for getting to this late. This looks like a bug - thanks for finding it! I guess it's not only for non-nullable types - list types could also cause a problem here, no?

I can't remember why exactly we're looking up a type here. I'm assuming this type lookup is there because we want to validate if the type name from item.detail is in fact existing in the schema provided. I think a correct fix is to determine if a type is a "wrapping type" and deal with each case properly. Thankfully it seems like we already have helper methods to work with these!

Would you be interested in creating PR for the fix maybe?

cc @schickling

[acao]

@timsuchanek thanks for your fix for this, I'm going to consider this resolved

[acao]

@yoshiakis is this not resolved yet? i think i mistook the PR above as having solved this

[yoshiakis]

@acao This issue is not solved yet. Sorry for stopping fixing this issue. Can I continue to be responsible to solve this issue? I think I can throw fixed PR in 1 or 2 weeks.

[acao]

would love your help, you are right on track already, that timeline sounds perfect!

[acao]

@yoshiakis this is in [email protected] which now has graphql 15 support! enjoy

[yoshiakis]

@acao would you please reopen this issue? Unfortunately, this issue hasn't been fixed yet. I'm currently modifying the test code in a branch which needs to be merged to fix this issue.

[acao]

@yoshiakis i merged as is, and I can add more tests in another PR, is that ok?

[yoshiakis]

@acao yeah, it's ok. and thank you for merging! I want to do other PRs for refactoring and adding test cases and changing code of hint.js a little bit because the way how the value of documentation in completion list is determined has been slightly changed by the merging, which didn't consider #1541.

[acao]

@yoshiakis @timsuchanek do we feel this bug is resolved now?

[yoshiakis]

@acao yeah, i think so :) (refactoring the test cases still remains, but the bug has been fixed so I think this issue can be closed.)

[acao]

speaking of which, do you have any reccomendations for getting flow + typescript working side by side in vscode? i can't seem to get it working without temporarily disabling Check JS for the entire workspace. it's been an issue since the typescript conversion

[acao]

also, thank you @yoshiakis so very much for your stewardship of codemirror-graphql!

[yoshiakis]

unfortunately, i don't know either. it's a pain to disable Check JS every time, though :(

[acao]

closing now, if any new issues with this arise with codemirror-graphql, please open a new bug ticket! thanks everyone for all your help on this!