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

Is FileParser safe? #54

Closed
Js-Brecht opened this issue Mar 9, 2020 · 2 comments · Fixed by #71
Closed

Is FileParser safe? #54

Js-Brecht opened this issue Mar 9, 2020 · 2 comments · Fixed by #71

Comments

@Js-Brecht
Copy link
Contributor

Js-Brecht commented Mar 9, 2020

In my investigations while looking for a good way to let gatsby-plugin-typgen know about extra queries for parsing, I ran into an issue.

I think it may not be safe to use FileParser. I was doing some testing to see what I could come up with without having to hook into Gatsby's internals, and I was using your processor. It took me a minute to realize that I was on Gatsby v2.17.11, where FileParser keeps the needed data under a definitions property... so I had to do this:

         if (extractionResult) {
-            const rawSdl = Array.isArray(extractionResult)
-                ? extractionResult.map(result => result.text).join('')
-                : extractionResult.text;
+            const rawSdl = Array.isArray(extractionResult.definitions)
+                ? extractionResult.definitions.map(result => result.text).join('')
+                : extractionResult.definitions.text;
             const document = parseGraphQLSDL(componentPath, rawSdl, { noLocation: true });
             trackedSource.set(componentPath, document);
         }

Since FileParser is internal, there is not a requirement for it's API to remain stable. Which could result in additional maintenance in your plugin, to keep it in sync with Gatsby.

Would it be safer to loop over components and staticQueryComponents from the state in onPostBootstrap? components contains page components, and staticQueryComponents contains the other components that have static queries, like the name suggests.

I think those both have the data you need. The componentPath property has the physical file location, and the query property has the raw query data. Perhaps I am missing something, though.

This would eliminate redundant file parsing, too, yeah?

@cometkim
Copy link
Owner

Yep, I had considered that.

It used FileParser to track Gatsby's early babel extraction because it doesn't tell me what the query is about. (After that, I don't use it at all)

And I thought optimistic that it would be consistent with Gatsby's inner workings and probably stable while Gatsby is v1.

I will gonna replace it with my own parser (that is regexp I already have in insert-type worker) in the next version and manage the test cases myself.

image

@Js-Brecht
Copy link
Contributor Author

It used FileParser to track Gatsby's early babel extraction because it doesn't tell me what the query is about. (After that, I don't use it at all)

I think I see where you're coming from. Since the state returned by Gatsby only includes components/queries that it will be using to fetch data from the GraphQL store, it doesn't include queries or fragments from plugins (or other places)... which would end up breaking some of the extra functionality you provide (like emitPluginDocuments, that type of thing). Basically, there's just not enough context.

I will gonna replace it with my own parser (that is regexp I already have in insert-type worker) in the next version and manage the test cases myself.

very cool 👍 🚀

There may be some small issues, however. For example, Gatsby accepts static queries as a variable:

const query = graphql`
   query {
      /* some query detail here */
   }
`

export const Foo = () => {
   const data = useStaticQuery(query);
}

I assume you already have plans for those use cases, though, so I'll stop and let you do your (fantastic) work 🙂.


I'll close this issue, since you've answered my question. 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.

2 participants