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

<Link /> doesn't work with hashes #5161

Closed
trusktr opened this issue Sep 14, 2018 · 15 comments
Closed

<Link /> doesn't work with hashes #5161

trusktr opened this issue Sep 14, 2018 · 15 comments
Assignees

Comments

@trusktr
Copy link

trusktr commented Sep 14, 2018

This is the same as #2098, except for me it is backwards.

#2098 says

it will only work when that link is pressed from another page.

When this link is pressed on the / page, Next will not scroll into that view

But what I experience is that it doesn't work from another page, and only works when on the page.

For example if I have <Link href="/#who-we-are" /> and I am on /about, clicking it takes me to /#who-we-are but will not scroll me to the section. Once I am at /#who-we-are I can click the link a second time to make it take me to the section.

@trusktr trusktr changed the title <Link /> doesn't handle hashes well <Link /> doesn't work with hashes Sep 14, 2018
@trusktr
Copy link
Author

trusktr commented Sep 14, 2018

I was able to fix it with a simple workaround. I've added the following to my code which seems to work, making it scroll to the section whenever the route has a hash:

    Router.onRouteChangeComplete = () => {
      setTimeout(() => {
        if (location.hash) location = location
      }, 0)
    };

I hope this doesn't have any implications. So far everything seems to work well, forward/back browser buttons seems to work as expected.

@trusktr
Copy link
Author

trusktr commented Sep 14, 2018

Also another problem: when clicking different <Link />s that vary only by hash, it does not trigger a route change event (onRouteChangeComplete).

This is unexpected because it means it will not cause some components to re-render if they are observing route changes. For example I want to highlight different menu items based on hash, and Next Router isn't telling me when they change, and neither is Window's hashchange event firing.

Any change to the browser history should be handled by Next.js router, so code can listen to it using Next.js Router API.

@trusktr
Copy link
Author

trusktr commented Sep 14, 2018

window.addEventListener('hashchange', console.log) doesn't work while using Next.js Router, all the more reason for Next.js to trigger onRouteChangeComplete for hash changes.

If I click on a <Link> that has a hash, there's no hashchange event fired. I can only trigger it by doing location.hash = 'someValue', which means I would have to step outside Next.js Router and implement something else.

It would be great for hashes to be first-class citizens in Next.js Router!

@trusktr
Copy link
Author

trusktr commented Sep 14, 2018

Looks like the answer is #1609, "not supported".

What does Next recommend to do then?

@arunoda said

for that you need to get the hash directly from the browser. location.hash

I know I can access the variable, but the problem is neither Next nor window hashchange event is telling me when the value has changed.

I could do something like polling, but that's not ideal.


I tried adding onClick to my element inside the Link (thanks to #4474), but when the element is clicked the hash hasn't changed yet, because routing is async, so I can't read the hash inside an onClick handler.


MDN says

Note that pushState() never causes a hashchange event to be fired, even if the new URL differs from the old URL only in its hash.

That's not helpful!


Is there another way?

@trusktr
Copy link
Author

trusktr commented Sep 14, 2018

I see in #4676 some work has gone into hashes.

What should we expect from Next.js regarding hashes currently?

@trusktr
Copy link
Author

trusktr commented Sep 14, 2018

For now, I'm doing the following to react to hash changes, which requires to specify for each nav link which hash it will change to:

import * as React from 'react'
import Router from 'next/router'

class MyComponent extends React.Component {
  constructor(props) {
    super(props)
    this.state = { path: '' }
  }

  render() {
    const {path} = this.state
    return (
              <Link href="/#someHash">
                <a
                  onClick={this.hashChangeTo('#someHash')} // specify which hash it changes to, even if no hash ("")
                  className={classNames({
                    foo: path.includes('#someHash'),
                  })}
                >
                  Go to Somewhere
                </a>
              </Link>
    )
  }

  hashChangeTo = (targetHash: string) => {
      return () => {

          // if we clicked a link that takes us to the same hash, nothing to do.
          if (targetHash === location.hash) return

          const detectHash = () => {
              if (location.hash === targetHash) {
                  // the hash changed to the expected target
                  this.updatePath()
              }
              else {
                  requestAnimationFrame(detectHash)
              }
          }

          requestAnimationFrame(detectHash)
      }
  }

  componentDidMount() {
    this.updatePath()
    Router.onRouteChangeComplete = this.updatePath
  }

  componentWillUnmount() {
    Router.onRouteChangeComplete = () => {}
  }

  updatePath() {
    this.setState({ path: Router.pathname + location.hash }); // Router doesn't have a .hash property

    setTimeout(() => {
        if (location.hash) location = location // scroll
    }, 0)
  }

}

All of this would not be needed if Next could have #5163.

@trusktr
Copy link
Author

trusktr commented Sep 14, 2018

Other libs (f.e. Vue router) trigger a route change event when the hash changes, making this a little simpler.

The following is the same thing in Vue:

<template>
            <router-link
              to='/#someHash'
              :class="path.includes(#someHash) ? 'foo' : ''"
             >
               Go to Somewhere
             </router-link>
</template>
<script>
    export default {
        watch: { $route() { this.updatePath() } },
        data: () => ({ path: '' })
        methods: {
            updatePath() {
                this.path = this.$router.history.current.fullPath
                if ( this.path.includes( '#' ) ) location = location // scroll
            },
        },
    }
</script>

Both Vue and Next need the location = location trick, though it'd be nice if they didn't. Next needs the more awkward polling for location.hash.

@trusktr
Copy link
Author

trusktr commented Sep 14, 2018

I was also thinking of trying this trick, but I haven't yet: https://stackoverflow.com/questions/4570093/how-to-get-notified-about-changes-of-the-history-via-history-pushstate

It'd be the best for Router to fully encompass hashes.

@timneutkens
Copy link
Member

Before reading all of this issue I'd like to point out the issue template is pretty clear right 🤔 Please use it when creating issues. It saves us a lot of time: https://github.com/zeit/next.js/issues/new/choose

@martpie
Copy link
Contributor

martpie commented Nov 20, 2018

Just faced this problem. Digging a bit into the Next.js codebase, I noticed it was supposed to be handled by Next.js itself here: https://github.com/zeit/next.js/blob/9547e77820305ac90a60d652f09ab29af53e11d3/packages/next/pages/_app.js#L49.

It can happen that scroll to hash does not work if you have a custom _app.js.

The (undocumented) solution now:

// app.js

// Container will scroll to hash on update
import App, { Container } from 'next/app'; 


...
class MyApp extends App {
  render() {
    const { Component } = this.props;
    // your you other stuff here

    return (
      <Container>
        <Component />
      </Container>
    );
  }
}

export default MyApp;

@timneutkens
Copy link
Member

@martpie I wonder if we should deprecate/remove Container and move it up the tree outside of userland 🤔

@martpie
Copy link
Contributor

martpie commented Nov 20, 2018

I understand the separation of concern, but in the end, without documentation, developers will never understand what happens there.

Would you accept a PR for that? (probably a breaking change though, unless you consider undocumented things are not things that can break 😄 )

@timneutkens
Copy link
Member

timneutkens commented Nov 20, 2018

@martpie we can make _app.js a lot simpler by rewriting a few bits, basically refactoring out all the logic around React context / the Container that implement scrolling, then what's left is "just" a React component. This would also be future-proof since React is moving to nearly fully functional style with Hooks and _app.js currently depends on class inheritance.

In doing that we can also implement React 16.3 context instead of using the old context API. (Breaking as we drop below 16.3 React support in doing so, potentially also React alternatives support, would have to check how preact/inferno handle React.createContext. Edit: seems like preact doesn't support it currently: preactjs/preact-compat#475)

@timneutkens
Copy link
Member

Created this issue wrt React context: #5716

@ijjk
Copy link
Member

ijjk commented May 5, 2019

This appears to be resolved and we have a test for this behavior, so I am going to close this issue. If you are still experiencing this feel free to reply with more info.

@ijjk ijjk closed this as completed May 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators May 5, 2020
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

4 participants