fix(gatsby): Wrong route resolved by findPageByPath function

[tlgimenes]

Description

This PR uses reach router's best matching route algorithm to resolve pages in findPageByPath function so we resolve to the best matching route instead of the first match.

Motivation

I'm playing around with Gatsby v4 and the File System Route API I ended up writing a project with the following pages directory:

./src/pages/
├── 404.tsx
├── 500.tsx
├── [...].tsx
├── [slug]
│   └── p
│       └── index.tsx
├── index.tsx

Both [...].tsx and [slug]/p/index.tsx pages use SSR rendering mode.
My intended behavior with this routing layout is for routes like /banana/p and /apple/p to be resolved to the [slug]/p/index.tsx page and routes like /ananas or /ananas/yellow to be resolved to the [...].tsx page.

I noticed that when running gatsby build && gatsby serve, the project worked as intended, but when running gatsby develop or when deploying it on both Netlify and Gatsby Cloud, my /banana/p and /apple/p where being resolved to the [...].tsx page.

Looking on both Netlify and Gatsby implemetations, I've noticed that, for resolving pages, they use this findPageByPath function. This function was not respecting reach router's best matching algorithm which led to the wrong page being resolved to.
This PR fixes this problem by using reach router's pick function. A drawback of this solution is that it may lead to some performance regressions since we parse and pick the best route for each function call. However, I believe the impact should be minimal since usually the number of routes to match should not be huge (~10 routes).

I'm open for other solutions and I hope this PR serves as a guide to fix this problem.
Thanks!

[KyleAMathews]

This makes sense!

Could you add some new tests that show how this fixed routing for the scenarios you described that don't currently work?

[tlgimenes]

Hey, thanks for the reply @KyleAMathews.

I wasn't really sure where to add the tests so I added them on e2e-tests/production-runtime at the SSR section. I added the following pages layout to this e2e test:

$ tree -L 4 ./e2e-tests/production-runtime/src/pages/ssr/path-ranking/
./e2e-tests/production-runtime/src/pages/ssr/path-ranking/
├── [...].js
└── [p1]
    ├── [p2].js
    └── page.js

On cypress, I added tests that open the following path and check if they resolve to the right file:

visit /p1 -> resolve to file [...].js
visit /p1/p2 -> resolve to file /[p1]/p2].js
visit /p1/page -> resolve to file /[p1]/page.js
visit /p1/p2/p3 -> resolve to file [...].js

Finally, I checked that the new version works correctly, whereas the master version fails on these tests.

Let me know if you think on different solutions or different places to put those tests. Thanks!

[tlgimenes]

Hi @pieh, any news about this one?

Let me know if you need any changes, Thanks!

[heynemann]

Just to add some color to this PR, this unlocks large enterprise clients (think very large multi-country enterprises) for VTEX. Our team bet on Gatsby and we're super happy with it. This PR would allow the team to move forward with migrating our clients to Gatsby V4, thus further improving their experience.

Is there anything we can do to help with the reviewing process? We're here to help!

Thanks for all your hard work and for providing such an incredible tool. We're very excited and confident in our bet in Gatsby.

[pieh]

This looks correct, I will just expand our existing unit tests as well.

While I will very likely merge as-is. This will need some iteration in the future to make it more performant and just consume less memory (memory usage note is actually more about what we already have than changes in this PR). When findPageByPath is used by engines (DSG / SSR) and we "fall back" to matchPath handling we do load all routes from LMDB store into memory and this is not great in general. Instead we should make use of match-paths.json we generate in https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/src/bootstrap/requires-writer.ts (actually it might be good idea to write out 2 variants of it - one that we have that also contain overlapping static pages - needed for browser runtime and new one that just contain match-paths ordered by specifity). This way we never will have to pull all the pages in memory with engines (maybe this can also be used with non-engines usages, but that becomes more tricky and we already have pages in memory then, so it's just CPU overhead and this is only used in gatsby develop IIRC)

[pieh]

Thank you @tlgimenes for implementing the fix and adding E2E test cases to make sure it works and there will be no regressions in the future!

[pieh]

This change was just released in @next channel (gatsby@4.5.0-next.4) and will be available in next stable release (~week from now)