redirect icon indicating copy to clipboard operation
redirect copied to clipboard

Incompatible with `Phoenix.Router.scope/{2,3,4}`

Open jvantuyl opened this issue 4 years ago • 9 comments

When inside of a scope, the macro that expands to Redirect is namespaced inside of the new scope.

For example, if I were to have this in my router.ex:

scope "/", MyAppWeb do
    pipe_through :browser

    redirect "/", "/landing", :temporary
end

Then I get a compile warning like (and a similar error at runtime):

warning: MyAppWeb.Redirect.init/1 is undefined (module MyAppWeb.Redirect is not available or is yet to be defined)
  lib/my_app_web/router.ex:23: MyAppWeb.Router.__checks__/0

jvantuyl avatar Feb 18 '21 01:02 jvantuyl

Could this be noted in the Docs. I've used Redirect in other projects and keep forgetting about this little "issue". A hint like this:

  redirect "/", "/directories", :temporary
     
  scope "/", ExampleWeb do
    pipe_through :browser     
#    live "/", PageLive, :index    
    live "/directories", DirectoryLive.Index, :index
    ....
  end

Also, awesome little plugin. It would be nice if Phx included the ability in the Router.

blackham avatar May 26 '21 15:05 blackham

I updated the readme in order to highlight that limitation. I think there's no way to make that work. Especially that we'd expect the path (to redirect to) to be relative to the one defined in the scope.

mathieuprog avatar May 27 '21 15:05 mathieuprog

Actually, taking the example in the issue, what do you think about being able to create MyAppWeb.Redirect, which would contain something like:

defmodule MyAppWeb.Redirect do
  use Redirect
end

Secondly, if the scope is /foo, and we have:

redirect "/bar", "/baz", :temporary

Should /foo/bar redirect to /baz or to /foo/baz. For me I'd expect the latter, but then what if you want to redirect out of the scope.

mathieuprog avatar May 27 '21 16:05 mathieuprog

Regarding question 1: I could work with that.

Regarding question 2:

In a perfect world, I'd expect it to use the rules for "relative URIs" as originally described in RFC 1808 and defined more thoroughly in RFC 3986.

Specifically, in §4.2, where:

  • a leading / makes gives a location relative to the root-path of the current Base URI
  • a schema-qualified full URL specifies an absolute location
  • a bare segment is relative to the current BaseURI
  • a rare special case of a "dot segment" sometimes is needed for relative paths that include a colon in the first segemtn

So, in that mythical world, baz would redirect to the current scope and /baz would redirect to a root scope.

This is congruent with what would happen if you returned that directly in a redirect, I believe. That said, it's not congruent with what Phoenix does (at least insofar as you tend to see leading slashes on all of the path segments). And the subtle behavior around colons in the initial path segment might be something you'd wish to avoid anyways.

jvantuyl avatar Jun 23 '21 09:06 jvantuyl

@mathieuprog Did this ever go anywhere? Can I make a PR for this? Or is this not something you care to do?

jvantuyl avatar Jul 02 '22 10:07 jvantuyl

Sorry for the late reply.

it's not congruent with what Phoenix does

It will then not be very readable with all the bunch of URLs starting with "/" under a scope...

How about just not allowing to have relative URLs to the root under a scope? Because it's still very limited, for example, how do you redirect a URL under scope A to a URL under scope B. Again, you'll have to write the full path of the URL under scope B, which loses all the benefits with what we're trying to do... Does it then make any sense to allow relative URLs to the root under a scope?

That's why I would suggest to have URLs under scope to just always be relative to that scope, without the possibility to "go up" in the route hierarchy.

Which leads me to another question, why do we need that (again considering the limitations that come with it)?

mathieuprog avatar Jul 15 '22 11:07 mathieuprog

Hi @mathieuprog ! thanks for this nice piece of code! I'm having the same issue mention here, but doing:

defmodule MyAppWeb.Redirect do
  use Redirect
end

returns an error:

** (UndefinedFunctionError) function Redirect.__using__/1 is undefined or private
    (redirect 0.4.0) Redirect.__using__([])

So not sure if someone (@jvantuyl) find a way to use the redirect within a scope? or if it's planned to add the defmacro __using__/1 on the module for this purpose?

cavi21 avatar Oct 29 '22 11:10 cavi21

Hi @cavi21 As mentioned in the readme, redirect doesn't work in scopes. Any snippet present in this issue would have been only suggestions for implementing the feature, but it didn't go anywhere.

The problem revolved around how to define scoped URLs and un-scoped URLs, scoped URLs redirecting to URLs in other scopes, etc. Therefore I think it's better to limit the redirections outside the scopes.

By the way, there's a new Sigil coming in Phoenix 1.7 for defining URLs, I wonder if this can address our issue here.

mathieuprog avatar Oct 29 '22 15:10 mathieuprog

Hi @mathieuprog, thanks for the followup and the explanation! make sense.

cavi21 avatar Oct 30 '22 08:10 cavi21