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

feat(gatsby): Handle duplicated fragment definitions #15179

Merged
merged 14 commits into from
Jul 1, 2019

Conversation

sidharthachatterjee
Copy link
Contributor

@sidharthachatterjee sidharthachatterjee commented Jun 27, 2019

Now that we run babel-loader for all dependencies after #14111, we should finally be able to support queries in dependencies!

This pull request adds node_modules as one of the folders to parse through. There are a couple of things to consider here though:

  1. Parsing through everything causes a GraphQL Error: RelayParser: Encountered duplicate defintitions for one or more documents: each document must have a unique name because of GatsbyImageSharpFixed etc (assuming these are coming from gatsby-plugin-sharp) My quick fix for this was filtering out any paths that contain /gatsby/ but of course we can't do that. How should we filter these out?

  2. This significantly slows down the extract queries from components phase since it now has to parse ~18000 files compared to like 10 before. One way to speed this up would be to filter deps by only ones that have gatsby as a dep or peer dep (since they would need the graphql export from gatsby to query)

Thoughts?

Todo

  • Better heuristic for filtering out duplicate fragments
  • Better filter to reduce number of files parsed

How do I test this?

Install gatsby-seo from npm (our SEO component extracted to a separate package) and use as a dependency.

Edit

We've reverted the addition to parsing node_modules because it depends on some other follow up work by @wardpeet. This PR just handles fragment duplication for now.

@sidharthachatterjee sidharthachatterjee requested a review from a team as a code owner June 27, 2019 08:11
@gatsbyjs gatsbyjs deleted a comment from gatsbot bot Jun 27, 2019
@KyleAMathews
Copy link
Contributor

One way to speed this up would be to filter deps by only ones that have gatsby as a dep or peer dep (since they would need the graphql export from gatsby to query)

Played a bit with this idea:

var rpt = require("read-package-tree")
rpt(
  __dirname,
  function(node, kidName) {
    // optional filter function– if included, each package folder found is passed to
    // it to see if it should be included in the final tree
    // node is what we're adding children to
    // kidName is the directory name of the module we're considering adding
    // return true -> include, false -> skip
    return /gatsby/.test(kidName)
  },
  function(er, data) {
    console.log({ data })
    console.log(data.children)
  }
)

This would require restricting queries to packages that include gatsby in their package name e.g. @sidharthachatterjee/gatsby-seo — which IMO is a reasonable restriction since the component wouldn't work outside of Gatsby.

@sidharthachatterjee sidharthachatterjee force-pushed the feat/support-queries-in-deps branch 2 times, most recently from 000d56e to 9abdcf9 Compare July 1, 2019 10:14
@sidharthachatterjee sidharthachatterjee force-pushed the feat/support-queries-in-deps branch from 9abdcf9 to 2069383 Compare July 1, 2019 10:52
@sidharthachatterjee sidharthachatterjee changed the title [WIP] feat(gatsby): Support queries in dependencies feat(gatsby): Support queries in dependencies Jul 1, 2019
@sidharthachatterjee sidharthachatterjee changed the title feat(gatsby): Support queries in dependencies feat(gatsby): Support queries in dependencies and themes Jul 1, 2019
@sidharthachatterjee sidharthachatterjee changed the title feat(gatsby): Support queries in dependencies and themes feat(gatsby): Handle duplicated fragment definitions Jul 1, 2019
Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌮

@sidharthachatterjee sidharthachatterjee added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Jul 1, 2019
@gatsbybot gatsbybot merged commit a92f6e1 into master Jul 1, 2019
@m-allanson m-allanson deleted the feat/support-queries-in-deps branch July 1, 2019 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants