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-plugin-sitemap): handle different query structures and allow custom siteUrl resolution #21948

Merged
merged 3 commits into from
Mar 12, 2020

Conversation

moonmeister
Copy link
Contributor

@moonmeister moonmeister commented Mar 4, 2020

Description

Resolves some of the issues discussed in #20692.

  • Previously when querying pages using allSitePage.nodes instead of allSitePage.edges was impossible because the serializer and internal page filter expected the edges object structure.

  • Previously you had to set your siteUrl at site.siteMetaData.siteUrl or the default resolver would fail. This seemed pointless when using other methods to resolve siteUrl. You can now define a function that receives the query result as "data", and you can return a string that will be used for siteUrl.

TBD

While this solution works there is a lot of "odd" things i had to do to make it backwards compatible. Specifically when filtering pages I convert the the data structure to an array of pages from either the edges or nodes structure. But it also still assumes use of allSitePage which kinda defeats the point of being able to write a custom query.

Logic has been added to handle both data structures. The absurd part is I have to return that data structure back to its original state to maintain backwards compatibility when handing the data back to the serializer. This applies to siteUrl resolution as well. Even if you write a custom resolver, accessing in in the serializer is still done at site.siteMetaData.siteUrl.

All this could be eliminated and the general api made more flexible and probably simpler, if we are okay with doing a major version bump.

Documentation

//TODO

Related Issues

closes #20692

@moonmeister moonmeister added type: bug An issue or pull request relating to a bug in Gatsby topic: plugins labels Mar 4, 2020
@moonmeister moonmeister requested a review from a team as a code owner March 4, 2020 07:10
@pieh
Copy link
Contributor

pieh commented Mar 6, 2020

All this could be eliminated and the general api made more flexible and probably simpler, if we are okay with doing a major version bump.

I think generally we are ok with doing major version bump for plugins (not tied to gatsby core major version bumps) with asterisk that breaking changes make sense and are thought out so we don't have to do another major version bump anytime soon to fixup problems in new API/options design.

In meantime I think we can live with a bit of "absurdity" (as you nicely described it :P)

@moonmeister
Copy link
Contributor Author

@pieh thanks. I'll finish docs changes this week and we can merge this as is. It sounds like if I felt like doing a more serious rewrite that'd be welcome. ✌️

@pieh
Copy link
Contributor

pieh commented Mar 11, 2020

Let me know if you want code review or should I wait for docs ;)

@moonmeister moonmeister requested a review from a team as a code owner March 11, 2020 13:59
@moonmeister
Copy link
Contributor Author

@pieh Got the docs in there. Thanks. I should have time for any needed changes this week.

Copy link
Contributor

@laurieontech laurieontech left a comment

Choose a reason for hiding this comment

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

Minor change, but docs look solid to me.

packages/gatsby-plugin-sitemap/README.md Outdated Show resolved Hide resolved
@pieh pieh self-assigned this Mar 12, 2020
@pieh pieh changed the title feat(plugin-sitemap): handle different query structures and allow cor custom siteUrl resolution feat(plugin-sitemap): handle different query structures and allow custom siteUrl resolution Mar 12, 2020
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.

Code looks good, let's get this in. Thanks @moonmeister!

@pieh pieh changed the title feat(plugin-sitemap): handle different query structures and allow custom siteUrl resolution feat(gatsby-plugin-sitemap): handle different query structures and allow custom siteUrl resolution Mar 12, 2020
@pieh pieh merged commit 07319d0 into master Mar 12, 2020
@delete-merged-branch delete-merged-branch bot deleted the sitemap-updates branch March 12, 2020 09:09
@pieh
Copy link
Contributor

pieh commented Mar 12, 2020

Published [email protected]

@vladar
Copy link
Contributor

vladar commented Apr 1, 2020

Apparently this PR contains some breaking change? #22703

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug An issue or pull request relating to a bug in Gatsby
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[plugin-sitemap] filter undefined
4 participants