RFC(plugin-sitemap): What would you like to see fixed or changed?

[moonmeister]

Summary

I recently did some work on gatsby-plugin-sitemap to resolve some edge case issues(#20692) caused by assumptions made in its code. This work fixed some issues and revealed some more. I made a note in the PR(#21948) that it might be time for a rewrite with some API changes to get all this smoothed out and to avoid some serious "jank" in the code.

Proposal

Basically, I have time since the world is on pause and might just get this done.

To start this process, I wanted to see if anyone out there had any suggestions on features to add or bugs to resolved in a new version(I'm expecting their to be breaking changes currently, but we'll see what happens)?

I'll do some thinking and design work for the API in the coming days and add what I'm thinking and incorporate any feedback I get. Thanks for any input!

[moonmeister]

Suggested new API,examples given would be defaults.

API Changes

LAST UPDATED: 2020/04/16
REVISION: B

// gatsby-config.js
module.exports = {
  siteMetadata: {
    siteUrl: `https://www.example.com`,
  },
  plugins: [
    {
      resolve: `gatsby-plugin-sitemap`,
      options: {
        output: `/sitemap.xml`, //UNCHANGED

        // UNCHANGED
        // Exclude specific pages or groups of pages using glob parameters
        // See: https://github.com/isaacs/minimatch
        // anny added categoies would be appended to these
        exclude: [
          `/dev-404-page`,
          `/404`,
          `/404.html`,
          `/offline-plugin-app-shell-fallback`,
        ],
        createLinkInHead: true, //UNCHANGED
        sitemapSize: Infinity, //UNCHANGED
        //UNCHANGED
        query: `
          {
            site {
              siteMetadata {
                siteUrl
              }
            }

            allSitePage {
              nodes {
                path
              }
            }
          }`
        ,
        /**
         * @name resolveSiteUrl
         *
         * UNCHANGED
         * @param {Object} data - results of the GraphQL query
         * @returns {string}
         */
        resolveSiteUrl: (data) => data.site.siteMetadata.siteUrl,
        /**
         * @name resolvePagePath
         *
         * NEW - This allows passing any data, it just has to end up in an array being
         * returned with "path" as the object property the uri.
         *
         * if you don't want to place the URI in "path" then resolvePagePath
         * are needed.
         *
         * @param {Object|string} page - Array Item returned from reolvePages
         * @returns {string}
         */
        resolvePagePath: (page) => page.path,
        /**
         * @name resolvePages
         *
         * NEW - This allows custom resolution of the array of pages.
         * This also where user's could merge multiple sources into
         * a single array if needed.
         *
         * @param {Object} data - results of the GraphQL query
         * @returns {Array}
         */
        resolvePages: (data) => data.allSitePage.nodes,
        /**
         * @name filterPages
         *
         * NEW - This allows filtering any data in any way.
         *
         * This Function is executed via allPages.map((page) => !excludes.some((excludedRoute) => thisFunc(page, ecludedRoute, tools)))
         * allPages is the results of the resolvePages
         *
         * @param {Object} page
         * @param {string} excludedRoute - Array from plugin config `options.exclude`
         * @param {Object} tools - contains required tools for filtering
         *
         * @returns {Array}
         */
        filterPages: (page, excludedRoute, { minimatch, withoutTrailingSlash, resolvePagePath }) =>
          minimatch(
            withoutTrailingSlash(resolvePagePath(page),
              withoutTrailingSlash(excludedRoute)
            )
          ),
        /**
         * @name serialize
         *
         * MODIFIED - the idea of passing specific siteUrl, and allPages values
         * instead of the rawQuery data is that devs would not need to rewrite
         * seralize, if the data structures changed. They would only need to change the
         * resolution functions and not how the data is being seralized.
         *
         * Hopefully this allow slower ejection from the defaults.
         *
         * This funciton would be exectuted after the array is filtered with the default or custom filter
         *
         * This funciton is executed by allPages.map(page => thisFunc(page, siteUrl, tools))
         * allpages is the result of the filter process
         *
         * @param {Object} page - results of the resolvePages function
         * @param {string} siteUrl - results of the resolveSiteUrl function
         * @param {Object} tools - contains tools for serializing
         *
        */
        serialize: (page, siteUrl, { resolvePagePath }) => {
          return {
            url: `${siteUrl}${resolvePagePath(page)}`,
            changefreq: `daily`,
            priority: 0.7,
          }
        },
      }
    }
  ]
}

Other Improvements

• Better error messages around failing to resolve Site URL to help users understand why it is required and how to resolve.
• errors around an failed resolutions that can be resolved by custom configs, and thus helping the user know when they need to eject.
• Update sitemap from 1.x --> 6.x

[moonmeister]

@pieh not sure if you have any input but you had requested a well thought out design process.

FYI: I'm starting to wonder if this will REQUIRE a major version bump. Changing the default query will require it; but, while it's a nice change IMO, it is not required. Other potential issues revolve around the changing of the serialize signature, but if we re-ordered them to keep the raw query data in the first position, we might be fine? It doesn't really matter ATM, we can always decide once we get into the code and see if other compatibility issues appear.

[pieh]

@pieh not sure if you have any input but you had requested a well thought out design process.

TBF, I don't have a lot of context and insight into sitemap plugin, but I will reserve some time tomorrow to go over your proposal and comments.

FYI: I'm starting to wonder if this will REQUIRE a major version bump. Changing the default query will require it; but, while it's a nice change IMO, it is not required. Other potential issues revolve around the changing of the serialize signature, but if we re-ordered them to keep the raw query data in the first position, we might be fine? It doesn't really matter ATM, we can always decide once we get into the code and see if other compatibility issues appear.

Comment like this just shows value in making research and design ;) We might not need major version bump in the end (tho this was never a blocker, just frequent breaking changes is just bad for DX and we should do everything we can to minimize those)

[moonmeister]

Writing this down before I forget...no guarantee of logical thought.

the custom resolvers are important but if someone is querying additional data for the serializer, and using that in a custom serializer...there is a good chance they don't want to bother with separately parsing the pages...they can probably just do it once in the custom resolvers? but If not in the resolvers...the filter still has to work. So maybe they have to do it twice or somehow merge the data from the additional query...

Well if they don't write a custom resolver...but to write a custom filter, then they don't need the custom resolvePages ...The output of the filter gets passed as allPages to the serializer...so they can handle things once in the custom filter and don't need the raw data possibly in the serializer.

The flexibility of this design allows for several solutions...so maybe we simply document clearly how and when each config gets executed...maybe a nice little flow chart. And devs can do what they think is best. This probably is not a code issue but just why docs need to be very clear.

[pieh]

FYI: I'm starting to wonder if this will REQUIRE a major version bump. Changing the default query will require it; but, while it's a nice change IMO, it is not required. Other potential issues revolve around the changing of the serialize signature, but if we re-ordered them to keep the raw query data in the first position, we might be fine? It doesn't really matter ATM, we can always decide once we get into the code and see if other compatibility issues appear.

If it would be possible to maintain current signature and just expand it (so users would only need to change their serialize implementation IF they make use of those new APIs) then this could be just minor bump. Given that current serialize signature have only 1 argument (object) with some fields - this should be doable as we would just add more fields there (or add them as 2nd/3rd params - but my gut feeling tells me it would be better to dump additional fields into 1st object - especially if those arguments would be optional to use - we could then avoid cases of user implementations looking like serialize: ({ site, allSitePage}, _notUsed, actuallyUsed) => { /* function body */}

[pieh]

I do like the look of those new APIs, but I have some questions:

In what scenarios resolvePages and resolvePagePath users would want to overwrite defaults? Querying allSitePage seems like catch-all solution (and possibility to filter pages with filterPages if user want to whitelist/blacklist some pages seems to me like it would handle the need to customize it in nice way (so it doesn't have to happen in serialize)).

I think you started writing it out in jsdoc descriptions for resolvedPages, but it seemed cut in the middle of sentence:

        /**
         * NEW - This allows custom resolution of
         *
         * @param {Object} data - results of the GraphQL query
         * @returns {Array}
         */
        resolvePages: (data) => data.allSitePage.nodes,

[pieh]

Additional thought:

What if instead of resolvePagePath there would be resolvePage where user is expected to return object with path, changefreq and priority (so pretty much shape of stuff that serialize return)?

(this might cause issues with backward compat tho - not sure - might depends on how much default options would change?)

Why? Users wouldn't have to call resolvePagePath themselves in serialize that way

[moonmeister]

In what scenarios resolvePages and resolvePagePath users would want to overwrite defaults?

Originally, the issue that was into and hackily fixed recently was some people querying allSitePage.nodes and others doing allSitePage.edges.node. But as demonstrated by the ghost cms starter...there may be others. I will note, the entire reason this is being done here is for unique sitemaps for different kind of content...I'll have to see if that's possible with this plugin and needs to be accounted for.

Querying allSitePage seems like catch-all solution (and possibility to filter pages with filterPages if user want to whitelist/blacklist some pages seems to me like it would handle the need to customize it in nice way (so it doesn't have to happen in serialize)).

I'm not sure if Iunderstand this question...but the reason I added these was so if a user wrote a custom query that broke defaults, they could override the resolvers without rewriting the entire serializer. Without the custom resolvers if you write a breaking query you would have to replace the filter function and the serialize function even if your output from those and their behavior is the same as the defaults. having the custom resolvers means you only have to customize the filter/serializer if you need to change their behavior, and not just the data structure.

What if instead of resolvePagePath there would be resolvePage where user is expected to return object with path, changefreq and priority (so pretty much shape of stuff that serialize return)?

Mostly because that doesn't fix the filter issue. Filter still has to be able to resolve the page path so you'd still need the custom resolver. What you suggest though makes me wonder if the serialize function is needed at all. I think it still makes sense to keep serialize in order to separate interest...not doing multiple steps in a single function allows the API to be more flexible.

TODO:

• research underlying sitemap generator api to see what capabilities we should maybe support.
• take a look a the advance sitemap generator to see if we should learn anything

[moonmeister]

I just realized it may be unnecessary to make the entire filter function be replicated. The custom filter could maybe be simply and only the function inside of allPages.filter...wondering if there's any down sides to this...not thinking so...and then we can just move the tools/excludes to be passed in and the new function

[masives]

I would also want you to consider one use case. When you're migrating into gatsby from another service you might be in situation where some of pages are server by gatsby while some are server from wordpress/anything else.

Currently you can only query for site and allSitePage. This is what I believe rawQueryData would allow to handle.

[moonmeister]

@masives Thanks for the comment, I hadn't thought of this kind of situation. While rawQueryData could be used for this...you comment made me realize this could also be handled by resolvePages(dev could merge multiple query structures if needed). Then the serializer would on need to act on allPages. That said this would require a major version bump cause currently the rawQuery data is the only thing passed to the serialize funciton...that said @pieh pointed out that we don't really want to create a situation where the function signatures regularly look like (_junk, siteUrl, allPages, { something }) => {} either.

Since we're ok with a major version bump I think maybe just changing the serialize function signature and default to:

      serialize: (page, siteUrl, {resolvePagePath}) => {
            return {
              url: `${siteUrl}${resolvePagePath(page)}`,
              changefreq: `daily`,
              priority: 0.7,
            }
          },

The allPages.map() aspect is completely obfuscated and required. If you need to customize what's in allPages you only option is to do it in resolvePages.

This would be inline with my comment above

The custom filter could maybe be simply and only the function inside of allPages.filter

Making the filterPages:

filterPages: (page, excludes, { minimatch, withoutTrailingSlash, resolvePagePath }) =>
            !excludes.some((excludedRoute) =>
              minimatch(
                withoutTrailingSlash(resolvePagePath(page)),
                withoutTrailingSlash(excludedRoute)
              )
            )
        ),

[moonmeister]

I've updated the proposal to include the above discussed changes. I like that it draws clearer lines between the different custom config options and each has a specific purpose as opposed to having lots of overlap with each other.

[moonmeister]

I just realized, if Gatsby prefixes paths devs may need to prefix paths from non gatsby sources. If a dev is pulling paths from outside Gatsby and also doing path prefixes they may need to access that in serialize. I'm thinking we could write a helper function that the siteUrl and also does the pathPrefix if needed. This would also mean we probably don't need to pass the siteUrl in directly as any custom or default functions could use this utility to prefix the domain and path prefixes as needed (yes I realize getting pages from allSitePage would already have Gatsby path prefix, this is only needed for custom serialize situations where an outside source is used). This would look something like

       serialize: (page, { resolvePagePath, prependUrlRoot }) => {
          return {
            url: prefixUrlRoot(resolvePagePath(page)),
            changefreq: `daily`,
            priority: 0.7,
          }
        },

prefixUrlRoot would check if the path already has the pathPrefix (e.g. /blog/) and add it if needed, it would prepend the siteUrl(e.g. https://gatsbjs.org/), and return the final url (e.g. https://gatsbjs.org/blog/my/other/page/).

[janosh]

Just came here from #25019 which led to me take a look at the gatsby-plugin-sitemap source. I fully agree that it could benefit from some serious reworking.

From reading this RFC, it's not quite clear to me why we'd need resolvePagePath, resolvePages and serialize. What's the benefit over doing it all in one function?

[moonmeister]

@janosh It's an escape hatch. If where you're fetching data from always stores the path on theobject.path then you'll never need to override the defaults (that's what's shown here in the RFC, the defaults). But if you have custom data source that stores the path in uri you can use this to override or even add logic to handle merged data source that may store the path in path and uri.

Notice that resolvePagePath is use in filter and other locations. you could manually re-write all these functions if you had data that didn't match the expected input. Being able to explicitly override this allows you to change the path lookup without rewriting the rest of the serialize and filter functions.

[janosh]

I see, thanks for taking the time to explain!

[moonmeister]

I'm hoping to start work on this soon. Any final comments or input? Then only thing I'm unsure on is how to handle pathPrefix, but that shouldn't be too hard to solve once I get writing code.

[janosh]

Looking forward to the update! 👍

[wardpeet]

I've created a canary version based on @moonmeister PR. Please try it out by running one of the two commands:

npm install gatsby-plugin-sitemap@next
yarn add gatsby-plugin-sitemap@next

[janosh]

@moonmeister @wardpeet Thanks for your work! Just tried out gatsby-plugin-sitemap@next. It's throwing the error

ERROR #98124 WEBPACK

Generating SSR bundle failed

Can't resolve './rx.lite' in 'node_modules/rx-lite-aggregates'

If you're trying to use a package make sure that './rx.lite' is installed. If you're trying to use a local file make sure that the path is correct.

File: node_modules/rx-lite-aggregates/rx.lite.aggregates.js

not finished Building static HTML for pages - 43.898s
error Command failed with exit code 1.

[moonmeister]

Hi @janosh!

Sorry to hear you're running into an issue. To help us best begin debugging the underlying cause, it is incredibly helpful if you're able to create a minimal reproduction. This is a simplified example of the issue that makes it clear and obvious what the issue is and how we can begin to debug it.

If you're up for it, we'd very much appreciate if you could provide a minimal reproduction and we'll be able to take another look.

Thanks for using Gatsby! 💜

[janosh]

@moonmeister Thanks for the quick reply! It's difficult to create a minimal repro as I'm unsure how to use the sitemap plugin. The next docs state differing function signatures. E.g. for serialize the default is said to be

serialize: ({ path, modifiedGmt }) => {
  return {
    url: path,
    lastmod: modifiedGmt,
  }
},

whereas the list of options says it's serialize(page, siteUrl, tools). Finally, looking at the actual PR, it seems to be serialize(page, { resolvePagePath }).

[moonmeister]

Hey good catch. So "the default is said to be" is not the defaults, it's an example and I'm using de-structuring to extract those fields from the page object. and not using the resolvePagePath provided. The list of options is incorrect though.,..good catch, I'll fix that, serialize(page, { resolvePagePath }) is indeed the correct signature. Thanks!

[janosh]

@moonmeister Thanks for the clarification. I read through the next docs some more. Maybe this sentence should be highlighted, perhaps as a blockquote instead of a sentence in a paragraph.

If you override the query, you may need to pass in a custom resolvePagePath, resolvePages to keep everything working. If you fetch pages without using allSitePage.nodes query structure you will definately need to customize the resolvePages function.

Also, small typo "definately" -> definitely.

Regarding the minimal repro, if I create a new barebones project

gatsby new bug-repro https://github.com/gatsbyjs/gatsby-starter-hello-world

and just yarn add gatsby-plugin-sitemap@next without any custom config

module.exports = {
  siteMetadata: {
    siteUrl: `https://example.com`,
  },
  plugins: [`gatsby-plugin-sitemap`],
}

it builds as expected.

On my blog, however, the same setup throws the above mentioned error,

Can't resolve './rx.lite' in 'node_modules/rx-lite-aggregates'

even after deleting and reinstalling node_modules

rm -rf node_modules && rm yarn.lock && yarn

Not sure how to debug that.

[moonmeister]

So we don't use rx.lite in sitemap, I'm skeptical this is an gatsby-plugin-sitemap@next issue. Can you confirm the site breaks by toggling the next vs current plugin? You may try updating the gatsby version or other dependencies to see if that resolves the issue as well.

[janosh]

Sorry, I wasn't able to get the current version of the sitemap plugin to work with my blog either as explained in #25019. But if I remove the next version from gatsby-config.js, the rx.lite error disappears and my site builds fine. This seems to indicate the issue is at least related to gatsby-plugin-sitemap.

[moonmeister]

@janosh Okay, I'll try and look into it but this is OSS work for me and I'm pretty busy atm...so not sure when I'll get to it.

[janosh]

No problem. I didn't mean to shove this off on to you. Will look into it more as well when I get the chance.

[moonmeister]

To his has been merged and published under @next to be released soon.