Move check for graphql string in source files to language plugin #2811
Conversation
…there are no additional checks done on the file content.
… JS language plugin where we know 'graphql' must be in the source for it to contain graphql tags.
There was a problem hiding this comment.
The language plugin already provides the findGraphQLTags function here:
https://github.com/facebook/relay/blame/master/packages/relay-compiler/bin/RelayCompilerMain.js#L279
What do you think of adding the getFileFilter function there as well?
This "early filter" was helping in reducing the number of files we keep track of in watch mode, skipping quickly over all the non-relay related files.
… RelaySourceModuleParser.
…efault getFileFilter function.
|
Hey @kassens , I think that sounds good! I made some changes - is this more in line of what you had in mind? |
…l-string-in-source-requirement
…l-string-in-source-requirement
|
cc @alunyov since you're on a merging spree ;) Would you mind having a look at this one too? It's the last piece of the puzzle to make the compiler to support what I need for making my Reason integration work without needing to maintain a fork. |
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.
Summary: This moves the check for the string `graphql` in source files to the JS language plugin, away from `RelaySourceModuleParser`. This has two reasons: 1. Language plugins might not require/use the string 'graphql'. I have that case when emitting ReasonML types - Writing GraphQL there looks like `[%relay.mutation ...]` and `[%relay.query ...]`, so `graphql` is not naturally inside any of the source files I want the compiler to pick up. 2. The requirement was quite "hidden" and it took me a few hours to understand why my files weren't picked up even though they seemed to satisfy all requirements of the compiler + configuration. So, also reducing confusion. If this lands I'll PR the TypeScript language plugin to add the same type of check into that as well. Pull Request resolved: #2811 Reviewed By: jstejada Differential Revision: D19856119 Pulled By: alunyov fbshipit-source-id: 864c75020db6993b0a3702c38a8ab22b7933073a
Hi @zth, I just wanted to remind you of this. Do you have time to update the TS language plugin? :) |
|
@mrtnzlml I actually believe this PR was changed enough from its original state that the TS plugin won't need PRing. Or have you encountered a bug because of this? |
|
I thought so, but I was mistaken - it was a different issue. Sorry for the useless ping. 😬 |
|
Haha no worries, just glad it works :D
…On Wed, Feb 19, 2020 at 7:57 AM Martin Zlámal ***@***.***> wrote:
I thought so, but I was mistaken - it was a different issue. Sorry for the
useless ping. 😬
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2811?email_source=notifications&email_token=AALD3WTGCQTNQVWBB2HTKNTRDTJ67A5CNFSM4IIJMH52YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMGSWOI#issuecomment-588065593>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AALD3WQL456RB4ITFHRYS2LRDTJ67ANCNFSM4IIJMH5Q>
.
|
This moves the check for the string
graphqlin source files to the JS language plugin, away fromRelaySourceModuleParser. This has two reasons:[%relay.mutation ...]and[%relay.query ...], sographqlis not naturally inside any of the source files I want the compiler to pick up.If this lands I'll PR the TypeScript language plugin to add the same type of check into that as well.