fix(plugin-catch-links): handle pathPrefix

[ahinrichs]

Remove pathPrefix before calling navigate, closes #8907

[pieh]

Should we add some tests for pathPrefix scenarios?

[ahinrichs]

I thought of the same. But I have little to no experience in writing effective tests. So someone else must jump in.

[JimLynchCodes]

hey, I have experience writing tests, and also I need this PR to be merged so my site will work. lol

...but I have zero familiarity with the gatsby codebase and not sure exactly what it is that needs to be tested and where to put the tests.

[JimLynchCodes]

this whole class is so abstract that I'm not sure what it is really doing, but I see that the PR really only changes two functions inside one file.

In the first chunk of changes you change pathIsNotHandledByApp to be a function that takes two parameters instead of one. Can it still be called with one? also, in the test for catch-links.js I don't see any mention of pathIsNotHandledByApp.

It probably would be nice to have something like this:

describe('pathIsNotHandledByApp works properly'. () => {

    it('works with some basic case'. () => {

       const fakeDestination = ""
       const fakepathStartRegEx = ""
       const expectedResult = ""  

        expect(pathIsNotHandledByApp(fakeDestination, pathStartRegEx).to.equal(expectedResult)


    })

})

[JimLynchCodes]

Also this. Why are we passing "jest.fn()" to the method routeThroughBrowserOrApp? Can we have more cases to run through routeThroughBrowserOrApp?

[ahinrichs]

Actually I can't think of any situation where this PR makes things worse. I'm just reusing the regex that was created inside pathIsNotHandledByApp. Therefore I create it in the callling function and pass it to pathIsNotHandledByApp as a parameter. The only semantic change is to replace /<pathPrefix>/ with / before calling hrefHandler if the catched link starts with it. So that navigate will get the unprefixed path as it expects. In builds without pathPrefix this is / to /. And in builds with pathPrefix things don't work at all ATM.

I wouldn't hold back the merge to wait for having tests. But it is a regression from #8289. And maybe tests would have helped to avoid this. Maybe merge and create an issue that this plugin needs more tests?

[ahinrichs]

Tried my very best™... This is a test that /pathPrefix/somePath href gets passed to hrefHandler as /somePath. This is what's currently right for gatsby-link.navigate. Better test would be to check that window.history recieves click on /pathPrefix/somePath as /pathPrefix/somePath.

[ahinrichs]

Found what I wanted in gatsby-link tests

[JimLynchCodes]

nice @ahinrichs. I like your idea for the test, but how can you know from the test when window.history recieves an event?

[ahinrichs]

For the best test one really should somehow mock window.history. This test is what gatsby-link.navigate - which is what is used by this module - finally passes to @reach/router.navigate. So it should be good enough to cover changes like #8289.

[ahinrichs]

After some thinking of it I think the test is more of an integration test. Anyone mind if I move it there?

[ahinrichs]

This was just a try. The integration test worked here but failed in circle.ci. So I reverted.

Actually I would like it a bit more as an integration test. The test failed for import { navigate } from "gatsby-link". I checked the circleci setup and it just calles gatsby-dev --scan-once --copy-all. When I try this locally gatsby-link and gatsby/cache-dir/navigation.js are copied. Dunno... Anyway. I'm happy with the fix and the tests.

[ahinrichs]

BTW, I always knew I will fall in love with writing tests the moment I start doing it :-)

[pieh]

will this test actually fail if you revert changes to src/catch-links.js? gotta say it's kind of confusing one to read ;)

[ahinrichs]

Just the one of them that simulates a build with --prefix-paths. That's the current state: module works in default builds but not in builds active prefix. But I wanted to catch the case too if any change would break the non prefix processing. As I said, these two are more of an integration test. I just don't know why I can't import "gatsby-link" there.

And yes, it was confusing. I tend to leave otherones code untouched. And this was a straight combination of the existing tests and one of the gatsby-link tests. I did some refactor and added comments.

[ahinrichs]

I created another PR #9051. With this circleci setup the test from commit 9e09c65 succeeds as an integration test (see #9050). Acutally we could still merge this PR in the actual state to get the plugin fixed and maybe--if I'm right with the circle ci setup--could change this PR tests the to integration tests in another step.

[chasemccoy]

@ahinrichs @pieh any ETA on getting this merged? This bug is causing some big headaches for me.

[pieh]

@chasemccoy I will be testing this locally today (mostly because those tests are kinda confusing to me :P)

[ahinrichs]

Just on last change to import { navigate } from "gatsby" instead of from "gatsby-link".

I'm pretty confident, that this PR first and most important fixes the issue and second the tests at least do no harm. So I think it could be merged.

The only thing--beside the new tests being a bit abstract--that is not ideal is the import from gatsby-link resp. gatsby. That way it isn't a pure unit test anymore. But that could be done in the next step.

@pieh If you feel uncomfortable with the tests, would it help if we just merge the change in the regular code?

[pieh]

@pieh If you feel uncomfortable with the tests, would it help if we just merge the change in the regular code?

No, we definitely want some kind of tests to catch potential regressions, I just need to dig a bit and see if it actually tests stuff ;)

[pieh]

So it tests correctly, little verbose but better to have this in (+fix obviously) and try to figure out nicer tests (or some refactoring to not repeat a lot of code there).

I just formatted code and skipped empty tests (we shouldn't report success on empty tests - this just gives false sense of coverage)

[chasemccoy]

Woohoo! Thank y'all for pushing this through, this will save me a lot of headaches.

[ahinrichs]

Actually - as an test beginner - I followed what was there. I first had a version where the test itself was in a function. But I assumed the code repetition in the tests before was for a reason. So I rewrote them to be two independent code blocks.

In the end I discovered the e2e test setup and I think this is best placed there. But this will take some days.

[ahinrichs]

The empty tests where to give the output some kind of "story telling" impression. They were already there. But skip doesn't make much sense to me. I would then prefer to remove them (and maybe rephrase some of the other texts to stand on their own).