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

feat: Improve redirects #79

Open
neznaika0 opened this issue Oct 14, 2024 · 10 comments
Open

feat: Improve redirects #79

neznaika0 opened this issue Oct 14, 2024 · 10 comments

Comments

@neznaika0
Copy link
Contributor

I continue to work with HTMX.
Should I redefine RedirectResponse(back(), route(), to()) and update redirect()?

$responseHTMX = single_service('redirectresponse')
        ->hxRedirect((string) previous_url(true))
        ->with(...$flashes)
        ;

$responseCI = redirect()->back()->with(...$flashes);

When the request came as HTMX, the redirect will have the initial headers. By making a regular redirect, the response will remain as for HTMX. Therefore, the second entry does not make sense.
Now we need to duplicate the code for different cases (HTMX or a regular request). Would it be better to combine these conditions inside the new functions?

@neznaika0
Copy link
Contributor Author

Also, I have not found a function for the relative path. It would be great to simplify hxLocation() when passing the full path:

$responseHTMX = single_service('redirectresponse')->hxLocation((string) previous_url(true));
public function hxLocation(...): RedirectResponse {
    // $path = 'http://example.local/admin/users?searchQuery=John&sort=ASC';
    /**
     * @var URI $uri
     */
    $uri  = single_service('uri', $path)->withScheme('')->setHost('');
    $data = ['path' => '/' . ltrim((string) $uri, '/')];
    // to '/admin/users?searchQuery=John&sort=ASC'
}

@michalsn
Copy link
Owner

The redirect() and hxRedirect() behave differently and set different headers.

If we send htmx request it doesn't mean automatically we will want to use hxRedirect() and vice versa.

I want to keep the current methods intact as they are the equivalent of the pure htmx implementation. However, I'm open to introducing some sort of helper function that would serve for auto-recognizing the type of request and then serving different responses.

function hxRedirect()
{
 // it htmx then this
 // and if normal request then that
}

@neznaika0
Copy link
Contributor Author

Yes, I mentioned the headers. This is the reason for the change.
It doesn't make sense to have a 30x redirect if the request comes from HTMX. It only stealthily replace content from new route. And it adds complexity if 30x is supposed to reload the page.

Example, from the list of users with the "user-list" fragment, click the Ban/Unban button. With HTMX, you need hxRedirect() / hxLocation() and return fragments, but without it (click on the link <a>) you need 30x redirects with the full page. Otherwise (now), after the 30x redirect, the page sends a fragment of the "user-list".

Therefore, duplicate conditions arise.

        if ($this->request->isHtmx()) {
            return $responseHTMX->with(...$flashError);
        }

        return redirect()->back()->with(...$flashError);

Changing the function would be a good solution to use a single code. hxRedirect() may help, but the code for the methods back()... also needs to be improved and hxRedirect() loses its meaning.

PS: Sorry for the machine translation, if not everything is clearly described

@michalsn
Copy link
Owner

You can name this helper function autoRedirect() or something similar - this can be discussed as we go.

Unfortunately, I will not approve any changes to the existing methods, because we need to be able to decide whether we use htmx response or not. This decision cannot blindly rely on the HX-Request header by default.

@neznaika0
Copy link
Contributor Author

I heard you. Maybe someday there will be users with a similar problem and we will come back to this. It's easier for me to modify my project

@neznaika0
Copy link
Contributor Author

#79 (comment)
Real improve?

@michalsn
Copy link
Owner

#79 (comment)
Real improve?

Yes, but I would implement it a bit differently:

public function hxLocation(...): RedirectResponse {

    if (str_starts_with($path, 'http')) {
        $path = (string) service('uri', $path, false)->withScheme('')->setHost('');
    }

    $data = ['path' => '/' . ltrim($path, '/')];
    
    // ...
}

Plus this would need a mention in the docs.

@neznaika0
Copy link
Contributor Author

There may be a problem with http verification if the URL is "http/users/123"

@michalsn
Copy link
Owner

Good point, you can change it to http:// and https://

@neznaika0
Copy link
Contributor Author

neznaika0 commented Oct 15, 2024

I won't be able to create a PR for the next few weeks.

I would not use a shared service, but get a new. There may be a conflict when the URI is used outside the redirect

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

2 participants