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): Match node manifest pages by page context slug #34790

Merged
merged 2 commits into from Feb 11, 2022

Conversation

TylerBarnes
Copy link
Contributor

This change allows Gatsby Preview (content sync) to work with more sites by default. Many Contentful sites use node.slug in page context to query for entries by their slug. This isn't good practise because slugs aren't unique in contentful (id's would be better), but it's done commonly enough that we can use it to match node manifests to pages for Content Sync routing. So this makes more Contentful sites work with Preview without needing to add an ownerNodeId

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Feb 11, 2022
@TylerBarnes TylerBarnes added topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine) and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Feb 11, 2022
@KyleAMathews
Copy link
Contributor

Curious here -- do we not use the resolved node id from a query for matching? E.g. if you query for a node by slug, you still get a node (maybe in this scenario not the node you want) which has a node id.

@TylerBarnes
Copy link
Contributor Author

TylerBarnes commented Feb 11, 2022

Yeah, GraphQL query tracking is our last fallback - this is because Gatsby doesn't differentiate between nodes queried on any single page. As humans we think of one node as owning pages - but querying for a next/prev link (or any other node data not directly related to the page being previewed) actually means there are 3 (or more) nodes attached to that page in query tracking.
So when we use only query tracking you're often routed to the wrong page. If you preview page B you could get to page A if page A has a query for some data from page B.

Our hierarchy for finding the right preview page is (in order of most specific to least specific):

  1. ownerNodeId property on createPage input
  2. Node used to create a filesystem route
  3. Node id in page context
  4. Slug in page context
  5. query tracking (least specific and least reliable)
  6. no page found

@KyleAMathews
Copy link
Contributor

Cool! Makes sense, thanks for the detailed explanation!

@KyleAMathews
Copy link
Contributor

Ok the next/prev, I imagine we could track those differently in the future to add an additional heuristic.

@TylerBarnes
Copy link
Contributor Author

Next/prev is just an example - any connected node or top level queries all look the same for query tracking - so if you have a list of top 10 latest blog posts, or a connection to an author, or to a list of related posts on a connection field, those all show up the same way. This is actually used to our benefit in Contentful because it allows us to route to any page where a nested content block is being queried, so you can press "Open Preview" on any content block at any depth and get to a page where you can view that content even if no top level page was created for it.

@KyleAMathews
Copy link
Contributor

no for sure. I'm saying a query like:

allBlogPosts(filter: { eq: $slug }) {
  nodes {
    id
    title
  }
  next {
    id
    title
  }
}

We'd mark the node in next as a lower priority than the node(s) in nodes.

@KyleAMathews
Copy link
Contributor

and dependencies in connection queries are always lower than querying a single node.

@KyleAMathews
Copy link
Contributor

Anyways, off-topic to the PR — just following a thought I had :-D

@TylerBarnes
Copy link
Contributor Author

Ahh, got it! 👌 Yeah I like that idea - we could definitely do some ranking within query tracking here in the future to make this work in more cases

DanielSLew
DanielSLew previously approved these changes Feb 11, 2022
Copy link
Contributor

@DanielSLew DanielSLew left a comment

Choose a reason for hiding this comment

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

Looks good and makes sense! The one thing I have a comment on is I think we should push towards making everyone use ownerNodeId's, rather than trying to make it work for as many edge cases as possible out of the box. Is this solution just specific to contentful?

packages/gatsby-cli/src/structured-errors/error-map.ts Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants