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

[v3] Add failing redirect browser test #5787

Merged
merged 4 commits into from Jul 24, 2023

Conversation

PhiloNL
Copy link
Collaborator

@PhiloNL PhiloNL commented Jul 20, 2023

I've added a failing test for: #5786

@vercel
Copy link

vercel bot commented Jul 20, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
livewire ❌ Failed (Inspect) Jul 20, 2023 8:38pm

@coleshirley
Copy link
Contributor

To add some of my findings to the original issue it seems that the issue is in js/features/supportNavigate.js

import { on, trigger } from "@/events"

let isNavigating = false

window.addEventListener('alpine:navigated', () => {
    isNavigating = true

    // Forward a "livewire" version of the Alpine event...
    window.dispatchEvent(new CustomEvent('livewire:navigated', { bubbles: true }))
})

export function shouldRedirectUsingNavigateOr(effects, url, or) {
    let forceNavigate = effects.redirectUsingNavigate

    if (forceNavigate || isNavigating) {
        Alpine.navigate(url)
    } else {
        or()
    }
}

the alpine:navigated event is dispatched once on page load and isNavigating is set to true and then never is set back to false. Thus all redirects from livewire are passed to Alpine.navigate()

@coleshirley
Copy link
Contributor

Further findings:
the line in dist/livewire.esm.js and dist/livewire.js that dispatches the alpine:navigated event is

setTimeout(() => {
   fireEventForOtherLibariesToHookInto();
});

which does not appear to be in the livewirejs or alpinejs source anywhere. So it may just be that a recompile of the js assets will fix the issue.

setting isNavigating back to false somewhere may be necessary as well though

@killjin
Copy link

killjin commented Jul 21, 2023

image

PHP:

$this->redirect($url);

This default use Alpine.navigate?

@joshhanley
Copy link
Member

@PhiloNL thanks! We also had another existing test that was failing due to this bug. See PR #5859 for details on why it is happening.

@joshhanley
Copy link
Member

@PhiloNL FYI test was failing because you need to add ->waitForLivewire() before any server call to ensure it waits for the Livewire request to finish before progressing with the test 🙂

@joshhanley joshhanley merged commit 660c077 into livewire:main Jul 24, 2023
4 checks passed
@joshhanley
Copy link
Member

Thanks!

@PhiloNL
Copy link
Collaborator Author

PhiloNL commented Jul 24, 2023

@PhiloNL FYI test was failing because you need to add ->waitForLivewire() before any server call to ensure it waits for the Livewire request to finish before progressing with the test 🙂

Thanks! Good to know 😄

@joshhanley
Copy link
Member

No worries! 🙂

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

Successfully merging this pull request may close these issues.

None yet

4 participants