search

Zaid-Ajaj / Feliz.Router

A router component for React and Elmish that is focused, powerful and extremely easy to use.
MIT License
78 stars 16 forks source link

Implement Base HREF handling for Paths #30

Closed BennieCopeland closed 1 year ago

BennieCopeland commented 3 years ago
Zaid-Ajaj commented 3 years ago

Hi @BennieCopeland, sorry for taking a while to review the code properly. Initially, I really liked the separation of the modules, it made the library somewhat cleaner for sure. However, on a second thought, I would like to avoid breaking changes as much as possible. Ideally, I would rather this library to not change its public API every couple of months and tell the users they have to keep up with all the possible methods and modules.

Instead, I suggest to simplify the implementation by only taking the <base href="..." /> into account when it is available in the document and when the path-routing mode is enabled. Basically a behind-the-scenes enhancements without having the users to change anything except for the version upgrade. Do you think this would be doable? If you don't feel doing a second iteration of the PR, just let me know so that I take the work over and put the needed pieces into the library.

In any case, thanks a lot for taking the time to implement this, really appreciate it 🙏

BennieCopeland commented 3 years ago

What you are suggesting is what I originally started with. The problem arises because of the shared urlSegments function. It needs to be aware of the base path in order to remove it prior to returning the segments. And encodeParts needs to be aware to add it back in. For testing, these functions need to remain pure, so I can't just add a side effecting call to document, which means it must be passed in. I originally looked at turning the RouteMode enum into a DU where RouteMode.Path took a string to carry the base path. This worked for urlSegments, but broke down in enocdeParts if I remember correctly. Another parameter could be added for the base href, but that led to the slack discussion on whether these functions were "implementation details" or "public" with people possibly relying on them. That is what prompted me to split it out into two modules, along with the benefit of being able to unify the Router and Cmd member names. However, the original Router.fs file has not been modified, and this would not break existing users. The new modules would be the recommended way for new solutions, or existing users needing the new base path functionality.

I started working on updating the readme yesterday and was thinking about the examples to add. The base href only applies to relative URLs, and the URI spec (RFC 3986) provides the method for resolving relative URLs in section 5.2. It differentiates between absolute and relative URIs based on whether the path starts with a "/" or not. I was looking at adding examples for path routing to show that an absolute URI of [ "/home" ] => "/home" while a relative path will resolve as [ "home" ] => "/somebase/home". But when I compared against the hash router, it gives [ "/home" ] => "#/home" instead of what it should be (in my opinion), "/home". I was looking at making the hash routing consistent with the path routing. Ideally, the user should not have to think about whether they are using hash or path based routing when encoding their URIs, but instead know that [ "/home"; "user" ] is an absolute URL and [ "home"; "user" ] is a relative URL. This would be a change for hash routing, but since it is in a new module, it's not really breaking unless you migrate to the new module.

This also kind of leads into talking about degenerate routes like [ "/home"; "/users" ] which gives /home//users. I was thinking something like head :: tail -> head @ tail |> removeLeadingSlashes |> removeTrailingSlashes if we really want to care about it. But that is probably going too far into trying to protect the user from silly mistakes.

Zaid-Ajaj commented 3 years ago

The problem arises because of the shared urlSegments function. It needs to be aware of the base path in order to remove it prior to returning the segments. And encodeParts needs to be aware to add it back in. For testing, these functions need to remain pure, so I can't just add a side effecting call to document, which means it must be passed in.

Why isn't it possible to make the two functions take an extra parameter and only execute the DOM side-effect at the very edge of the call site?

nother parameter could be added for the base href, but that led to the slack discussion on whether these functions were "implementation details" or "public" with people possibly relying on them.

I don't think people should be relying on urlSegments or encodeParts and if they are, that is not what this library is for. Another parameter is most likely the way to go.

. Ideally, the user should not have to think about whether they are using hash or path based routing when encoding their URIs, but instead know that [ "/home"; "user" ] is an absolute URL and [ "home"; "user" ] is a relative URL. This would be a change for hash routing, but since it is in a new module, it's not really breaking unless you migrate to the new module.

That's right, here are the cases:

BennieCopeland commented 3 years ago

Why isn't it possible to make the two functions take an extra parameter and only execute the DOM side-effect at the very edge of the call site?

It is possible, as I did it in the PathRouter.fs file. There was just my concern with someone relying on their current implementations.

That's right, here are the cases:

  • When <base href="/home" /> is present then urlSegements "/home/user" should give [ "user" ] because that is relative to the base url.

  • When <base ... /> is not present or simply href="/" then urlSegements "/home/user"should give [ "home"; "user" ]

For path routing, this makes sense and is what the new code does. For hash routing, whatever is set in <base href> is irrelevant because hash routing uses window.location.hash where "http://localhost/myspa" will return a hash location of "".

What I was referring to is when using Router.navigate, Router.format, or Cmd.navigate. You get two different answers when using a leading slash, and I think they should be the same answer.

The first is an absolute path, which correctly ignores the base path and is equivalent to "http://localhost/home" while the latter is a relative path that is equivalent to "http://localhost/myspa/#/home". I think they should both output "/home", and that the first segment is recognized as determining whether the path specified is an absolute path from the root domain, or a relative path within the application.

I still think that two separate modules provides a better end user experience for the following:

Zaid-Ajaj commented 3 years ago

What I was referring to is when using Router.navigate, Router.format, or Cmd.navigate. You get two different answers when using a leading slash, and I think they should be the same answer.

Yeah about those leading slashes: the idea with the functions Router.navigate, Router.format, or Cmd.navigate is that they accept route segments. So really when someone asks for Router.navigate [ "/home" ] in path mode, then that is just Router.navigate [ "home" ] and it should respect the <base href="/myspa" /> ultimately navigating to http://localhost/myspa/home

So I believe we are on the same page when it comes to the behaviour of the library. As for the public API, I am not a big fan at this moment especially of "Your choice is controlled by the import, not the member name" I know it is very subjective but please bear with me on this one :pray: 🙇

I admit the proposed public API indeed makes it easier to switch between the route modes but that is an optimization of an already fairly easy task. Yes, it does involve more find and replace to make the switch but that alone IMO doesn't justify changing the API or introducing a new one just yet. Will take it into account within a future release

BennieCopeland commented 3 years ago

I will update the existing API with the base href handling as requested.

Zaid-Ajaj commented 3 years ago

Thanks a lot @BennieCopeland 🙏 really appreciate it and looking forward to it (no rush though :smile:)

Zaid-Ajaj commented 1 year ago

closing as stale. if there is still interest in this pr, consider reopening 🙏