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

Active class not being added to NavLinks on active pages on v4.3.1 #6201

Closed
dotspencer opened this issue Jun 9, 2018 · 44 comments
Closed

Active class not being added to NavLinks on active pages on v4.3.1 #6201

dotspencer opened this issue Jun 9, 2018 · 44 comments

Comments

@dotspencer
Copy link

dotspencer commented Jun 9, 2018

Version

Upgraded react-router-dom from 4.2.2 → 4.3.1

Node v8.11.2
npm v5.6.0

With the following dependencies:

"babel-core": "^6.26.3",
"babel-loader": "^7.1.4",
"babel-preset-env": "^1.7.0",
"babel-preset-react": "^6.24.1",
"react": "^16.4.0",
"react-dom": "^16.4.0",
"react-router-dom": "^4.3.1",
"webpack": "^4.10.2",
"webpack-cli": "^3.0.1"

Steps to reproduce

After upgrading, the 'active' class is no longer applied to the <NavLink/> for active pages. I've tried explicitly adding the activeClassName prop to the component and it still doesn't add the class the the element.

I haven't been able to create a minimal verifiable example. Has anyone else seen this? Everything else works perfectly fine, builds like normal, but the active class isn't added on the updated version.

Expected Behavior

It works as expected with v4.2.2 but not with v4.3.1 (or v4.3.0)

Additional Info

v4.2.2

Props being passed to NavLink:

activeClassName: "active"
ariaCurrent: "true"
children: Array[2]
className: "img-link"
exact: true
to: "/"

JSX and HTML:

<NavLink exact={true} to="/" className="img-link">
 ↓
<a class="img-link active" aria-current="true" href="#/">


v4.3.1

Props being passed to NavLink:

activeClassName: "active"
aria-current: "page"
children: Array[2]
className: "img-link"
exact: true
to: "/"

JSX and HTML:

<NavLink exact={true} to="/" className="img-link">
 ↓
<a class="img-link" href="#/">

@timdorr
Copy link
Member

timdorr commented Jun 9, 2018

Can you put something together on codesandbox.io? This needs a repro to be diagnosed.

@dotspencer
Copy link
Author

I wasn't able to reproduce it on codesandbox or a barebones app I created. Both worked fine using the same dependency versions...

@jaredLunde
Copy link

I'm encountering the same issue w/ 4.3.1, but likewise have not been able to reproduce it in a simple app - will be sure to update here if I can get a basic reproduction.

@jameswyse
Copy link

jameswyse commented Jun 12, 2018

I'm also seeing this after upgrading from 4.2.2 to 4.3.1

Workaround: pass a function to the isActive prop to do the check manually, eg:

const isActive = (path, match, location) => !!(match || path === location.pathname);

<NavLink to="/my/path" 
         className="nav-link"
         activeClassName="nav-link--active"
         isActive={isActive.bind(this, '/my/path')}>
  Click Me 
</NavLink>

@timdorr
Copy link
Member

timdorr commented Jun 12, 2018

Can anyone create a reproduction of this on codesandbox.io or just point to a repo with the issue? There really needs to be more code to look at to diagnose what's going on. My best guess is the new path escaping, but that's purely a guess at this point.

@lotap
Copy link

lotap commented Jun 12, 2018

@timdorr I'm running into the issue as well. By using the react-devtools extension, I could see that the Route returned by NavLink has an extra backslash at the beginning of the path.

You can test it in your browser console

// https://github.com/ReactTraining/react-router/blob/ae35a273bdb0fdd2fec32893ed1a62630da8bf3b/packages/react-router-dom/modules/NavLink.js#L25
'/foo'.replace(/([.+*?=^!:${}()[\]|/\\])/g, "\\$1")

// using OSX returns "\/foo" in Chrome or "\\/foo" in FireFox + Safari idk about windows/ie

@jonthomp
Copy link

We've had this issue reported by someone using tabler-react.

There is reproducing codesandbox here. If we move the SiteWrapper component into the Example and Dashboard components, rather than wrapping Switch, the problem goes away.

@pshrmn
Copy link
Contributor

pshrmn commented Jun 15, 2018

I just took a quick look at those two sandboxes and the broken one works when I refresh it, which is weird but I don't know if it has anything to do with the issue. If I take out the tabler-react components and just render <NavLink>s, it works, too.

If someone can put together a barebones (just React Router and host components) sandbox that duplicates the issue, it would be appreciated.

@dagda1
Copy link

dagda1 commented Jun 16, 2018

Same issue here with 4.3.1, I have an ssr app and the class is being added from the code rendered on the server but not when changing views on the client.

I rolled back to 4.2.2 and all is now good.

I tried to debug and match was always null in NavLink at this location.

@timdorr
Copy link
Member

timdorr commented Jun 16, 2018

I put something together here that doesn't recreate the issue: https://codesandbox.io/s/0p78xv2v5p In NavLink.js, I put both the 4.2 and 4.3 versions of NavLink in there. Both work just fine...

I'm now wondering if it has to do with #5964.

@Mati365
Copy link

Mati365 commented Jun 16, 2018

I think its related to #6193.

@dagda1
Copy link

dagda1 commented Jun 16, 2018

So when I was debugging and it is the first time I've really looked at the code, I stripped it back to one route on "/". It initially found a match but then it seemed to render another route for the children of the component defined in the route with a weird \/ route for which it could not find a match and so match was null in NavLink and hence no activeClass.

@pshrmn
Copy link
Contributor

pshrmn commented Jun 17, 2018

@dagda1 Can you throw that stripped back code into a sandbox? Or is this only an issue for you with SSR?

@dagda1
Copy link

dagda1 commented Jun 17, 2018

@pshrmn I'm pretty sure this is only ssr related

@jonthomp
Copy link

@pshrmn @dagda1 the issue is not SSR related for us

@pshrmn
Copy link
Contributor

pshrmn commented Jun 18, 2018

@jonthomp I just took a closer look at tabler-react and the <Nav.Item> is a pure component, so when the location changes, the <NavLink>s aren't being re-rendered (the blocked updates problem). That will go away when RR adopts the new context, but isn't the same issue as other people are experiencing.

To everyone else in this thread: can someone please put together a small repo or sandbox?

@dotspencer
Copy link
Author

I created a small repo that reproduces the bug: https://github.com/dotspencer/react-router-bug

One curious thing I discovered is that the problem goes away when I move the express package from "dependencies" to "devDependencies" in package.json.

@pshrmn
Copy link
Contributor

pshrmn commented Jul 8, 2018

@dotspencer React Router's path-to-regexp dependency is v1.7.0 while Express's is v0.1.7. During installation, the path-to-regexp dependency for Express is installed before the one for React Router, so you end up with a node_modules that looks like this:

node_modules
  express
  path-to-regexp@0.1.7
  react-router
    node_modules
      path-to-regexp@1.7.0

Your Webpack configuration is set to resolve directly from the root node_modules, so when you build your project you are building it with the old path-to-regexp. The solution, based on this comment is to use a generalized node_modules reference in your Webpack config.

  resolve: {
    modules: [
      path.resolve('./src'),
-     path.resolve('./node_modules'),
+    'node_modules',
    ],
  },

If anyone else is running into this issue and uses Express + Webpack, can you please verify if this fixes your problem. (cc @dagda1)

@lotap
Copy link

lotap commented Jul 9, 2018

@dagda1
Copy link

dagda1 commented Jul 9, 2018

Absolutely worked for me also. I would never have suspected that.

I have a mono repo. Still not entirely sure how this fixes it for the monorepo.

mathieu-bellange added a commit to mathieu-bellange/project-nz that referenced this issue Sep 14, 2018
- correctif dans la config webpack qui entrainait un bug dans react-router (remix-run/react-router#6201)
@paustria
Copy link
Contributor

I'm not sure if this is related to this issue. But when I use relative path, the activeClassName is not applied.

For example,

<NavLink
    to="./test"
    activeClassName="is-active"
    exact={true}
>
     Test
</NavLink>

Active class is not applied.

<NavLink
    to="/test"
    activeClassName="is-active"
    exact={true}
>
     Test
</NavLink>

Active class is applied.

The url routes to the correct path but no active class.

@apvilkko
Copy link

apvilkko commented Oct 30, 2018

I ran into the same issue but could not use the generalized 'node_modules' workaround. Instead, I hard-coded path-to-regexp to always resolve to the newer version via alias.

resolve: {
  alias: {
    'path-to-regexp': path.resolve(
        __dirname, 'node_modules', 'react-router', 'node_modules', 'path-to-regexp'
    )
  },
  modules: {...}
}

@ashoksudani

This comment has been minimized.

@glebtv
Copy link

glebtv commented Nov 27, 2018

Escaping path in NavLink seems to be not needed as <NavLink to="/path"/> seems to be rendering <Route path="\/path" /> because of it

reverting this seems to fix it for me
2d80151

@yellareddy-m
Copy link

@dotspencer React Router's path-to-regexp dependency is v1.7.0 while Express's is v0.1.7. During installation, the path-to-regexp dependency for Express is installed before the one for React Router, so you end up with a node_modules that looks like this:

node_modules
  express
  path-to-regexp@0.1.7
  react-router
    node_modules
      path-to-regexp@1.7.0

Your Webpack configuration is set to resolve directly from the root node_modules, so when you build your project you are building it with the old path-to-regexp. The solution, based on this comment is to use a generalized node_modules reference in your Webpack config.

  resolve: {
    modules: [
      path.resolve('./src'),
-     path.resolve('./node_modules'),
+    'node_modules',
    ],
  },

If anyone else is running into this issue and uses Express + Webpack, can you please verify if this fixes your problem. (cc @dagda1)

Hi @pshrmn , is there a way to get this worked if we not using webpack.config but instead using create-react-app to setup the react app. is ejecting the app only way to this fixed in this case ?

@hannasdev
Copy link

Any news on this? I'm using a mono-repo with Express and Create-react-app so the only solution so far seems to be to eject, which I'd rather avoid (In that case it seems simpler to use the function in NavLink to do the comparison manually).

@osimek1
Copy link

osimek1 commented Jan 21, 2019

@GothBarbie it seams that this issue is fixed with 4.4.0-beta builds (I have switched to 4.4.0-beta.6)

NumminorihSF added a commit to Saritasa/react-app-core that referenced this issue Jan 23, 2019
@esbenvb
Copy link

esbenvb commented Feb 7, 2019

Until 4.4 is stable, I'm using this workaround... It's not ideal since I have to specify the path twice when calling the component.

const matchWorkaround = (pathname) => (isMatch, location) => isMatch || location.pathname.startsWith(pathname);

const AccountMenu: React.SFC<IProps> = ({ accountNumber, isAdvisor }: IProps) => (
    <div style={styles.wrapper}>
        <NavLink
            activeStyle={styles.active}
            style={styles.menuItem}
            to={`/banklink/accounts/${accountNumber}/current-allocation`}
            isActive={matchWorkaround(`/banklink/accounts/${accountNumber}/current-allocation`)}
        >
            Formuesammensætning
        </NavLink>
        <NavLink
            activeStyle={styles.active}
            style={styles.menuItem}
            to={`/banklink/accounts/${accountNumber}/deposit-allocation`}
            isActive={matchWorkaround(`/banklink/accounts/${accountNumber}/deposit-allocation`)}
        >
            Fordeling af nye indskud
        </NavLink>

@ghost
Copy link

ghost commented Feb 7, 2019

I'm not sure if this is related to this issue. But when I use relative path, the activeClassName is not applied.

For example,

<NavLink
    to="./test"
    activeClassName="is-active"
    exact={true}
>
     Test
</NavLink>

Active class is not applied.

<NavLink
    to="/test"
    activeClassName="is-active"
    exact={true}
>
     Test
</NavLink>

Active class is applied.

The url routes to the correct path but no active class.

@paustria were you able to solve this? the URL routes but no active class is applied...

@paustria
Copy link
Contributor

paustria commented Feb 7, 2019

@Ameer157 I'm currently solving by not using . or using next.js. I did a test on version 4.4.0-beta.6. It's still broken and it seems to be more buggy. Now, / path always show active class name on <NavLink>. There might be changes on how to use <NavLink> on a new version.

@ghost
Copy link

ghost commented Feb 13, 2019

Until 4.4 is stable, I'm using this workaround... It's not ideal since I have to specify the path twice when calling the component.

const matchWorkaround = (pathname) => (isMatch, location) => isMatch || location.pathname.startsWith(pathname);

const AccountMenu: React.SFC<IProps> = ({ accountNumber, isAdvisor }: IProps) => (
    <div style={styles.wrapper}>
        <NavLink
            activeStyle={styles.active}
            style={styles.menuItem}
            to={`/banklink/accounts/${accountNumber}/current-allocation`}
            isActive={matchWorkaround(`/banklink/accounts/${accountNumber}/current-allocation`)}
        >
            Formuesammensætning
        </NavLink>
        <NavLink
            activeStyle={styles.active}
            style={styles.menuItem}
            to={`/banklink/accounts/${accountNumber}/deposit-allocation`}
            isActive={matchWorkaround(`/banklink/accounts/${accountNumber}/deposit-allocation`)}
        >
            Fordeling af nye indskud
        </NavLink>

Hi @esbenvb,

I can't get this workaround to work for some reason, the active class is still bugged...

P.S. what is: style={styles.menuItem} ? 👍

@benjamingeorge
Copy link

I ran into the same issue but could not use the generalized 'node_modules' workaround. Instead, I hard-coded path-to-regexp to always resolve to the newer version via alias.

resolve: {
  alias: {
    'path-to-regexp': path.resolve(
        __dirname, 'node_modules', 'react-router', 'node_modules', 'path-to-regexp'
    )
  },
  modules: {...}
}

I thought this might be fixed in v5 but it is not. I still have to use the workaround shown above by apvilkko

@limeandcoconut
Copy link

limeandcoconut commented Mar 25, 2019

@esbenvb I'm having this problem too. Haven't tried it but couldn't you have matchWorkaround use the this.props.to of the NavLink for matching?

@alvaro1728
Copy link

Version 5.0.0 fixed this. Thanks!

@wojtekmaj
Copy link

wojtekmaj commented Jun 4, 2019

I don't see how this got fixed in 5.0.0. I have links like

<Link to="page" activeClassName="active">Link</Link>

which gets resolved (correctly) to

<a href="/products/product-name-1/page">Link</a>

and when I'm on /products/product-name-1/page, activeClassName is not added.

@dagda1
Copy link

dagda1 commented Jun 4, 2019

@wojtekmaj have you tried npm i path-to-regexp@1.7.0 -S

@wojtekmaj
Copy link

@dagda1 I have only one copy of path-to-regexp in my repo, and it's exactly 1.7.0.

@AkilaDPerera
Copy link

react-router-dom@5.0.1 is the latest working package in 06/09/2019. Use this package so that you can experience the behavior mentioned in the documentation.
activeClass problem is no more. :)

@dagda1
Copy link

dagda1 commented Jun 8, 2019

i had this problem on 5.0.1. i’m fairly sure it was getting clobbered by webpack-dev-server’s express reference

@eulow
Copy link

eulow commented Jun 11, 2019

Passing in a callback to isActive, the match argument is returning null, location is fine. activeClass and activeStyle are not working for me.

@dagda1
Copy link

dagda1 commented Jun 11, 2019

@eulow that is the behviour I experienced, I implemented my own isActive and noticed no matter what I did a match was not found. I even ran the latest path-to-regexp locally through the node console with the same arguments and a match was found.

Are you using react-scripts or create react app?

I think the path-to-regexp is still getting clobbered by webpack-dev-server's express reference in some circumstances.

Create react app is making react-scripts a dependency and not a devDependency.

This only happened on a production build.

my fix was

npm i path-to-regexp@1.7.0 -S

A match was then magically found.

@AkilaDPerera
Copy link

react-router-dom@5.0.1 is the latest working package in 06/09/2019. Use this package so that you can experience the behavior mentioned in the documentation.
activeClass problem is no more. :)

Hey, I was wrong. Again I ran into the same problem in a different project. This time I found the real fix. Update your "path-to-regexp" package to latest as mentioned by @dagda1.

@dagda1
Copy link

dagda1 commented Jul 13, 2019

don’t update to the latest package. update to

npm i path-to-regexp@1.7.0 -S

@o-alexandrov
Copy link

Please consider reopening the issue as the exact same reported issue happens when pre-rendering SPA also

@lock lock bot locked as resolved and limited conversation to collaborators Sep 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests