gatsbyjs / gatsby

The best React-based framework with performance, scalability and security built in.
https://www.gatsbyjs.com
MIT License
55.3k stars 10.31k forks source link

Match path's aren't correctly sorted #16098

Closed kirbycool closed 5 years ago

kirbycool commented 5 years ago

Description

Hi team! I've been encountering some inconsistencies with match path sorting. With two pages with match paths of /* and /txt/*, the /* page gets sorted before the /txt/* page. But if I include a /app/* page, that gets correctly sorted before /*.

This was addressed by @pieh in https://github.com/gatsbyjs/gatsby/pull/9719, but the fix seems incomplete.

Steps to reproduce

  1. Starting from the default Gatsby starter, create a src/pages/app.js and src/pages/txt.js, and create have a gatsby-node.js like this:

    exports.onCreatePage = ({ page, actions }) => {
    const { createPage } = actions
    
    if (page.path.match(/^\/app/)) {
    createPage({
      ...page,
      matchPath: "/app/*",
    })
    }
    
    if (page.path.match(/^\/txt/)) {
    createPage({
      ...page,
      matchPath: "/txt/*",
    })
    }
    
    if (page.path === "/") {
    createPage({
      ...page,
      matchPath: "/*",
    })
    }
    }
  2. Run gatsby build

  3. Check .cache/match-paths.json. It should look like this:

[
    {
        "path": "/app/",
        "matchPath": "/app/*"
    },
    {
        "path": "/",
        "matchPath": "/*"
    },
    {
        "path": "/txt/",
        "matchPath": "/txt/*"
    }
]

Note: Changing the /txt/ page's matchPath to /app/* doesn't change the ordering. It looks like matchPath isn't taken into account or is superseded by some other sorting.

[
    {
        "path": "/app/",
        "matchPath": "/app/*"
    },
    {
        "path": "/",
        "matchPath": "/*"
    },
    {
        "path": "/txt/",
        "matchPath": "/app/*"
    }
]

Expected result

/txt/* should be sorted before /* because it is more specific (more path segments)

Actual result

/txt/* is sorted after /*

Environment

System: OS: macOS 10.14.5 CPU: (8) x64 Intel(R) Core(TM) i7-7820HQ CPU @ 2.90GHz Shell: 3.0.2 - /usr/local/bin/fish Binaries: Node: 12.3.1 - ~/.asdf/shims/node Yarn: 1.16.0 - ~/.asdf/shims/yarn npm: 6.9.0 - ~/.asdf/shims/npm Languages: Python: 2.7.15 - /Users/thanx/.asdf/shims/python Browsers: Chrome: 75.0.3770.142 Safari: 12.1.1 npmPackages: gatsby: ^2.13.39 => 2.13.39 gatsby-image: ^2.2.6 => 2.2.6 gatsby-plugin-manifest: ^2.2.4 => 2.2.4 gatsby-plugin-offline: ^2.2.4 => 2.2.4 gatsby-plugin-react-helmet: ^3.1.2 => 3.1.2 gatsby-plugin-sharp: ^2.2.9 => 2.2.9 gatsby-source-filesystem: ^2.1.6 => 2.1.6 gatsby-transformer-sharp: ^2.2.4 => 2.2.4

pieh commented 5 years ago

Thanks for reproduction steps!

It was broken afterwards in refactoring, and we didn't have tests to validate regressions. Will submit fix (again :S) and be sure to add e2e tests for this

gatsbot[bot] commented 5 years ago

Hiya!

This issue has gone quiet. Spooky quiet. πŸ‘»

We get a lot of issues, so we currently close issues after 30 days of inactivity. It’s been at least 20 days since the last update here.

If we missed this issue or if you want to keep it open, please reply here. You can also add the label "not stale" to keep this issue open!

As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! πŸ’ͺπŸ’œ

kirbycool commented 5 years ago

@pieh did this get fixed?

jhoffmcd commented 5 years ago

@kirbydcool, @pieh I think I just hit this problem on the latest release. I'm going to test a few things and come up with a minimal reproduction.

jhoffmcd commented 5 years ago

Here is a reproduction with just some client-only paths and gatsby-plugin-intl: https://codesandbox.io/embed/gatsby-matchpath-issue-test-zqzx2?fontsize=14

Note that /en-us/account route works while /en-us/account/profile is serving up the 404 page. This is because the matchPath for that 404 page is /en-us/* and is above the /:locale/account/* rule in the match-paths.json file:

[
    {
        "path": "/en-us/404/",
        "matchPath": "/en-us/*"
    },
    {
        "path": "/es-es/404/",
        "matchPath": "/es-es/*"
    }
    {
        "path": "/en-us/account/",
        "matchPath": "/:locale/account/*"
    },
    {
        "path": "/es-es/account/",
        "matchPath": "/:locale/account/*"
    }
]

NOTE if you replace :locale with actual values for locale like en-us, you still get the same outcome.

wardpeet commented 5 years ago

Fixed in gatsby@2.15.8

kirbycool commented 5 years ago

Awesome thank you!