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

window.location holds incorrect value on initial client-side render #430

Open
Tommypop2 opened this issue May 26, 2024 · 4 comments
Open

Comments

@Tommypop2
Copy link

Describe the bug

When client-side navigating to a different route, the window.location property still holds the data for the previous route. This means that any components using this data display the incorrect value for the current route.

This can be worked around by putting the window.location call inside of a setTimeout with a delay of about 20ms, but this is a pretty non-ideal workaround

Your Example Website or App

https://github.com/Tommypop2/router-bug-repro

Steps to Reproduce the Bug or Issue

Go to index route.
Press "About" button to client-side navigate to the "about" route.
Observe "window.location.href" still being from the previous route (http://localhost:3000/)

Expected behavior

One would expect to see the location property being up-to-date with the current route.

Screenshots or Videos

image

Platform

  • OS: [e.g. macOS, Windows, Linux]
  • Browser: [e.g. Chrome, Safari, Firefox]
  • Version: [e.g. 91.1]

Additional context

No response

@Tommypop2 Tommypop2 changed the title window.location.href holds incorrect value on initial render window.location holds incorrect value on initial render May 26, 2024
@Tommypop2 Tommypop2 changed the title window.location holds incorrect value on initial render window.location holds incorrect value on initial client-side render May 26, 2024
@Brendonovich
Copy link
Contributor

I think this is intentional - /about is evaluated before the navigation is finished, otherwise any resources the route might suspend on wouldn't be part of the navigation transition.

@Legend-Master
Copy link
Contributor

I think we probably should document this, currently if you change the title on render, the browser history's title will be filled with the new title instead of the old one, this is even the case for solid docs

image

Example code:

import { MetaProvider, Title } from '@solidjs/meta'
import { Router, Route, A, useLocation, Navigate } from '@solidjs/router'
import { JSX } from 'solid-js/jsx-runtime'
import { render } from 'solid-js/web'

function App(props: { children?: JSX.Element }) {
    const location = useLocation()
    return (
        <MetaProvider>
            <Title>{location.pathname}</Title>
            <nav>
                <div>
                    <A href="/a">A</A>
                </div>
                <div>
                    <A href="/b">B</A>
                </div>
            </nav>
            <main>{props.children}</main>
        </MetaProvider>
    )
}

render(
    () => (
        <Router root={App}>
            <Route path="/" component={() => <Navigate href="/a" />} />
            <Route path="/a" component={() => <h1>A</h1>} />
            <Route path="/b" component={() => <h1>B</h1>} />
        </Router>
    ),
    document.getElementById('root')!
)

Needs to be something like

function App(props: { children?: JSX.Element }) {
    const location = useLocation()
    const isRouting = useIsRouting()
    const title = createMemo<string | undefined>((prev) => {
        return isRouting() ? prev : location.pathname
    })
    return (
        <MetaProvider>
            <Title>{title()}</Title>
            <nav>
                <div>
                    <A href="/a">A</A>
                </div>
                <div>
                    <A href="/b">B</A>
                </div>
            </nav>
            <main>{props.children}</main>
        </MetaProvider>
    )
}

@ryansolid
Copy link
Member

ryansolid commented Sep 25, 2024

When in doubt assume SolidMeta is wrong. I do think this behavior in the router is intentional. We don't update the route until the Transition is complete. Some things leak like SolidMeta but that is incorrect. If that Meta information depended on something async it shouldn't show it before it is ready. Meta tag handling is notoriously handled poorly in pretty much all libraries. I've had some ideas to improve it but we lack the primtives today. This is actually a great example of its shortcomings. The reason though is that Meta tags get rendered outside of the tree so techniques we have to contain side effects while rendering don't apply to it today.

What happens when we do a Transition in routing is we pre-emptively load the next page before we leave the current page. That is the mentality we want to preserve here. We want to hold navigation until we get to the next page. Maybe the URL should happen on the front side instead of the backside of navigation because it never relies on async but I think that is inconsistent with the model.

Now unfortunately browser based navigation pushes the event first and doesn't give us the choice and in those cases things are less than ideal. But we have a back cache to help with that so in most cases there is no delay.

@johndunderhill
Copy link

johndunderhill commented Sep 27, 2024

We have learned the hard way not to rely on window.location in the router. For example, if you set the title for the new page before navigation is complete, it gets attached to the old page in the browser history (and the title for the new page never gets set).

The page title in the browser history is wrong when navigating to some pages

Previously we just deferred setting the title about 100ms. We are now experimenting with the update to isRouting() mentioned above. But we ended up refactoring everything that looks at window.location to call a library routine that can mitigate problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants