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

fix(gatsby): Fix client side routing match paths regression #15010

Merged

Conversation

Moocar
Copy link
Contributor

@Moocar Moocar commented Jun 21, 2019

Fix for #14993. The issue was that I accidentally removed a check whether the URL matches the page's matchPath before explicitly navigating. I fixed and hopefully made the logic a bit easier to follow.

@Moocar Moocar force-pushed the fix-client-side-routing-match-paths-regression branch from f914b24 to 9f0a039 Compare June 21, 2019 06:19
@Moocar Moocar force-pushed the fix-client-side-routing-match-paths-regression branch from 9f0a039 to 8a71503 Compare June 21, 2019 06:20
@wardpeet
Copy link
Contributor

Tested the PR with examples given on the issues and using examples/client-only-paths. It seems to work. Also tested it with prefix-paths 👌 . Some tests would be great.

@sidharthachatterjee sidharthachatterjee changed the title Fix client side routing match paths regression fix(gatsby): Fix client side routing match paths regression Jun 21, 2019
@sidharthachatterjee
Copy link
Contributor

Added some end to end tests to this!

@sidharthachatterjee sidharthachatterjee marked this pull request as ready for review June 21, 2019 09:52
@sidharthachatterjee sidharthachatterjee requested a review from a team as a code owner June 21, 2019 09:52
e2e-tests/serve/gatsby-config.js Outdated Show resolved Hide resolved
e2e-tests/serve/gatsby-config.js Outdated Show resolved Hide resolved
e2e-tests/serve/gatsby-config.js Outdated Show resolved Hide resolved
e2e-tests/serve/package.json Outdated Show resolved Hide resolved
packages/gatsby/cache-dir/loader.js Show resolved Hide resolved
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

I added some comments about making the e2e-test assets a bit smaller. Might speedup build time. I also wonder if this needs to be a different e2e test rather than just adding these tests to production-runtime. As of now I'm okay with just creating a new one.

@sidharthachatterjee should we add this to circleci? 👀 I'm not sure they are run.

@rchrdnsh
Copy link

ETA until this fix is merged? Looks like it only needs one more test to pass...

...currently dead in the water on a site until this is fixed, sooooo...

...what's the web dev equivalent of having to pee really badly but stuck in a long line for the bathroom?

🥺

@wardpeet
Copy link
Contributor

@rchrdnsh you can always lock gatsby on version < 2.9.0

@rchrdnsh
Copy link

rchrdnsh commented Jun 21, 2019

no version seems to work for me now...I've locked to 2.8.3, 2.8.6, 2.8.8, and I think a couple others, but none work for me, for some reason...🤷‍♂️

I might be doing it wrong, however. I go into package.json and change the gatsby version number, then delete and re-install my npm modules, and as well as delete the cache. Is this ok? Is there a better way to do it?

@wiziple
Copy link
Contributor

wiziple commented Jun 21, 2019

@rchrdnsh better do it using command because your gatsby version is already locked by package-lock.json or yarn.lock.

The other workaround is just removing existing lock file and then installing again.

@rchrdnsh
Copy link

rchrdnsh commented Jun 22, 2019

hmmmm...never messed with the lock file before...can I just delete that file then npm install?

@wardpeet
Copy link
Contributor

wardpeet commented Jun 24, 2019

@rchrdnsh yes you can.

You could install gatsby@resource-loading to get a version with this fix Sorry this is not for this PR, it's something else.

@sidharthachatterjee
Copy link
Contributor

This is good to go now. I've removed the new e2e test suite and instead have added tests for these in the existing suites, namely development-runtime and production-runtime

@freiksenet freiksenet added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Jun 24, 2019
@gatsbybot gatsbybot merged commit f52bbac into gatsbyjs:master Jun 24, 2019
@sidharthachatterjee
Copy link
Contributor

Published in gatsby@2.10.4

johno pushed a commit to johno/gatsby that referenced this pull request Jul 17, 2019
…#15010)

* export loader.findMatchPath

* reorganize production-app explicit navigate if statement

* strip BASE_PATH prefix before finding matchPath

* Add end to end tests

* Remove screenshots

* Update tests

* Add .gitignore

* Move tests to development runtime

* Hard code path in tests

* Remove serve e2e tests

* Add tests in production runtime
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants