IDE friendly errors #2752
IDE friendly errors #2752
Conversation
| @@ -110,7 +110,7 @@ function createCombinedError( | |||
| `${prefix}Encountered ${errors.length} error(s):\n` + | |||
| errors | |||
| .map(error => | |||
| String(error) | |||
| (error instanceof GraphQLError ? printError(error) : String(error)) | |||
There was a problem hiding this comment.
Use our own formatter for GraphQLError instances, instead of the one from graphql-js.
| return printedLocations.length === 0 | ||
| ? error.message | ||
| : [error.message, ...printedLocations].join('\n\n') + '\n'; | ||
| } |
There was a problem hiding this comment.
All only needed so we can use our own copy of highlightSourceAtLocation.
| @@ -23,14 +23,14 @@ import type { | |||
| GraphQLTagFinder, | |||
| } from '../language/RelayLanguagePluginInterface'; | |||
|
|
|||
| const cache = new RelayCompilerCache('RelayFindGraphQLTags', 'v1'); | |||
| const cache = new RelayCompilerCache('RelayFindGraphQLTags', 'v2'); | |||
There was a problem hiding this comment.
Have to bust the cache, as we should now cache the entire tag, instead of only the tag.template.
| memoizedTagFinder(text, baseDir, file).forEach(tag => { | ||
| const source = new GraphQL.Source( | ||
| tag.template, | ||
| path.join(path.relative(process.cwd(), baseDir), file.relPath), |
There was a problem hiding this comment.
Keep path short by making it relative, but in a way that the IDE can find it (i.e. relative to the working dir).
|
It strikes me that all of the code in RelayCompilerError that’s imported from graphql-js is only done because it isn’t exported upstream. I would rather have these file/line locations formatted correctly for IDEs anywhere graphql-js is used, so would you be open to my trying to get those exported upstream and remove them from our codebase? (I made a upstream PR for the formatting regardless graphql/graphql-js#1949) |
|
Yay! ❤❤❤ |
alloy
force-pushed
the
ide-friendly-errors
branch
from
cfb7d04
to
0af981e
Compare
|
Unsure why the ‘push’ build failed, but it’s due to some git branch issue. |
There was a problem hiding this comment.
@alunyov has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@alloy thank you for this PR. The demo looks super awesome! We may need more time to merge this internally, because this PRs, as is, is breaking our internal compiler setup. I'll follow up on the progress. |
@alloy Thanks for pinging me on Slack. I merged graphql/graphql-js#1984 that expose new |
|
@IvanGoncharov Great stuff, thanks! I’ll give it a try and let you know 👍 |
alloy
force-pushed
the
ide-friendly-errors
branch
from
300723f
to
1415b7d
Compare
|
@IvanGoncharov Follow-up is at #2784 |
|
@kassens No worries–no need ever to apologise for not having time to work on external OSS! So, semantically speaking this PR is still needed and separate from #2784. However, seeing as those changes also exist in #2784 and if you are ready to bump the version constraint of the In the meantime I'll rebase these PRs and I'll be available the rest of the day for further coordination on these. |
This is the format used by jest, flow, typescript, and many others.
Either an absolute path or a path relative to the wd is necessary for IDEs to find the file in question.
alloy
force-pushed
the
ide-friendly-errors
branch
from
1415b7d
to
025a41f
Compare
|
Since we're 100% focused on the compiler rewrite and this required some internal changes. I'll close this for now. Better IDE integration is one of our new goals :) |
These changes make it so file/line locations in errors can be made linkable by IDEs. This is the format used by jest, flow, typescript, and many others.
Before
After