fix: 52 use location usage

[StuartMorris0]

Fixes #52.

• adds react-router-dom dep
• updates to types and resolutions for react
• changes imports in index and test files
• might be worth updating the peerDeps if this is for react-router-dom mainly...

[icd2k3]

Thanks @StuartMorris0 !

Sorry, but you'll have to rebase from the last PR - this works for me, though!

[StuartMorris0]

Thanks @StuartMorris0 !

Sorry, but you'll have to rebase from the last PR - this works for me, though!

All done @icd2k3

[coveralls]

Coverage remained the same at 100.0% when pulling c86833c0c18ba16cc9192feec744bd6914ce3ba4 on StuartMorris0:fix/52-use-location into 0b9263512fccdeed1fe435b685d474f902845a3b on icd2k3:master.

[icd2k3]

@StuartMorris0 thanks! but I'm still confused why tests aren't passing for you 🤔 - out of curiosity, I updated react-router in this repo and all the unit tests run fine without that error... wondering what's different about your environment:

here's the 6.3.0 update: https://github.com/icd2k3/use-react-router-breadcrumbs/pull/55

[StuartMorris0]

@StuartMorris0 thanks! but I'm still confused why tests aren't passing for you 🤔 - out of curiosity, I updated react-router in this repo and all the unit tests run fine without that error... wondering what's different about your environment:

here's the 6.3.0 update: #55

@icd2k3 Your tests are a little different to ours, we use react-testing-library over enzyme so we render the actual component as the DOM would see it.

The tests are wrapped the same in a MemoryRouter.

Our breadcrumb component looks like this:

  const location = useLocation();
  const breadcrumbData = useBreadcrumbs(
    breadcrumbs
      ? breadcrumbs.map(({ label, dynamicLabel, backTo }) => ({
          path: backTo ?? location.pathname,
          breadcrumb: dynamicLabel ? label : Label,
          props: {
            label
          }
        }))
      : undefined,
    {
      excludePaths: !!breadcrumbs ? [] : ['/'],
      disableDefaults: !!breadcrumbs
    }
...

Our call to useLocation works fine as the import is from react-router-dom, whilst the useBreadcrumbs call fails as useLocation is from react-router.

The error refers directly to this repo.

useLocation() may be used only in the context of a <Router> component.
      at invariant (../packages/react-router/lib/router.ts:5:20)
      at Object.useLocation (../packages/react-router/lib/hooks.tsx:75:3)
      at useReactRouterBreadcrumbs (node_modules/use-react-router-breadcrumbs/dist/cjs/index.js:278:27)
      at Breadcrumbs (src/components/Breadcrumbs/Breadcrumbs.tsx:39:26)
      at renderWithHooks (node_modules/react-dom/cjs/react-dom.development.js:14803:18)

But you can see here we do have a MemoryRouter in the stack the component is rendered in.

    The above error occurred in the <Breadcrumbs> component:
        in Breadcrumbs (at Breadcrumbs.test.tsx:27)
        in Routes (at Breadcrumbs.test.tsx:22)
        in Provider (at test-utils.tsx:52)
        in Unknown (at test-utils.tsx:51)
        in Router (created by MemoryRouter)
        in MemoryRouter (at test-utils.tsx:47)
        in IntlProvider (at test-utils.tsx:37)

I'm fairly sure the import should be from react-router-dom. Not sure if you can update your tests to pick this up

[icd2k3]

@StuartMorris0 interesting - I updated my PR to convert unit tests to RTL (this is something I've been meaning to do anyways), but I don't see the same error.

https://github.com/icd2k3/use-react-router-breadcrumbs/pull/55

In your example, above breadcrumbs, if you import useLocation from react-router, does it work then?

I just want to make sure I fully understand why it might be happening since this change will require a major version bump (cause it could break people depending on how the use react-router vs react-router-dom and if they have the latter installed)

[StuartMorris0]

All our imports will complain in the same way if they're not from react-router-dom. It's react-router that throws the error.

[icd2k3]

@StuartMorris0 I can't reproduce that, though, which is confusing... I converted all my tests in the other PR to RTL and even tried adding useLocation to the component test locally like your example above (imported from either react-router or react-router-dom) and I don't get any errors.

[StuartMorris0]

I'll take a look at the other PR and play around

[icd2k3]

@StuartMorris0 - thanks - I'll add another commit now with useLocation as part of each test. Sorry for the trouble, just want to make sure I fully understand what might be happening before a major version bump

[StuartMorris0]

No worries. Always better to find something reproducible

[icd2k3]

The other thing I've been reading is that react-router is compatible with both web DOM and react native, so I'd want to make sure react native folks could still use the lib

[StuartMorris0]

Thanks for the feedback @icd2k3

I think we can close this out. I made a bunch of changes to our testing format wrappers. Even though testing-library was reporting that the router was there I think it was failing to create it.

I re-jigged our wrappers and managed to get it working, so the change is no longer needed. Thanks for pushing back to try and get this reproducible. We can close this out.