Open jerith666 opened 5 years ago
Hey @jerith666, if you want any help with this, I would like to see this upgraded.
Great! If you want to propose updates to the docs and examples, I can review and hopefully we can wrap this up! :)
FWIW I've been using git submodules to work with this in 0.19 while I await its completion. See https://github.com/jerith666/elbum/commit/49873d62c964fcadb2c03d640a09abb7bbce1ca3. It might be nice to see a second example of this working in a real app, via that mechanism, before we declare it done too.
Okay.
On Wed, Jan 16, 2019 at 9:13 PM Matt McHenry notifications@github.com wrote:
Great! If you want to propose updates to the docs and examples, I can review and hopefully we can wrap this up! :)
FWIW I've been using git submodules to work with this in 0.19 while I await its completion. See jerith666/elbum@49873d6 https://github.com/jerith666/elbum/commit/49873d62c964fcadb2c03d640a09abb7bbce1ca3. It might be nice to see a second example of this working in a real app, via that mechanism, before we declare it done too.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/rgrempel/elm-route-url/pull/38#issuecomment-455036301, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAXI_S38RsCwt9FuKjWDn9g0CGqF7I4ks5vD_h6gaJpZM4ZZZvG .
I've been shifted to other work, so I have not had a chance to look at this yet.
On Wed, Jan 16, 2019 at 9:24 PM Andrew Lenards andrew.lenards@gmail.com wrote:
Okay.
On Wed, Jan 16, 2019 at 9:13 PM Matt McHenry notifications@github.com wrote:
Great! If you want to propose updates to the docs and examples, I can review and hopefully we can wrap this up! :)
FWIW I've been using git submodules to work with this in 0.19 while I await its completion. See jerith666/elbum@49873d6 https://github.com/jerith666/elbum/commit/49873d62c964fcadb2c03d640a09abb7bbce1ca3. It might be nice to see a second example of this working in a real app, via that mechanism, before we declare it done too.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/rgrempel/elm-route-url/pull/38#issuecomment-455036301, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAXI_S38RsCwt9FuKjWDn9g0CGqF7I4ks5vD_h6gaJpZM4ZZZvG .
I realize this PR has been dormant for a long while, so maybe the momentum to land this is no longer there. However, I recently revived an old Elm 0.17 app and upgraded it first to 0.18 and then to 0.19.1 (using @jerith666 's fork as a submodule as suggested), so I thought I might share a bit of feedback around that.
I had a bit of a hard time figuring out why programWithFlags
requires passing an onUrlRequest
handler now and what to do in it exactly. To my (maybe naive) understanding, the RouteUrl
module would/should/could handle url request (like, clicking on a link that takes you to another page in your app etc.). Thus it should be enough to provide delta2url
and location2messages
, as we did in Elm 0.18.
I started with providing a no-op handler there, that is onUrlRequest = \urlRequest -> NoOp
, where NoOp
is a message of my app that does nothing in update
. With that in place, the app would compile, but of course, clicking on a link like <a href="#some-fragment">go somewhere else</a>
would now longer work now.
I think it would be more intuitive if at least internal UrlRequest
s would be handled directly in elm-route-url
, like this: https://github.com/basti1302/elm-route-url/commit/aeef0e7b645fae7360bb42b59f776ad53808aadb
This directly wraps internal url requests into a RouterMsg
(which is then implicitly fed to RouteUrl#update
), so the app does not need to take care of them.
I'm undecided on external UrlRequest
, maybe those still should be passed on to the application that is using elm-route-url.
An alternative solution would be leave the onUrlRequest
as it is now in this PR and just have the app turn it into a RouteMsg
on its own – maybe there apps with use cases that require additional actions to be taken even for internal URL requests? But then elm-route-url needs to expose a way to create RouteMsg
and feed them back into RouteUrl#update
. Actually, wrapLocation
already exposes a way to create those but:
a. its comment suggests that a client app is not actually supposed to use it (except for testing), and
b. I'm not sure how this could type check successfully as the app's onUrlRequest
needs to return the Msg
type of the app and can not return elm-route-url's WrappedMsg
type.
Of course, it is totally possible that I am holding this wrong and missing something obvious.
Thanks for the feedback!
I just looked through how I have onUrlRequest
implemented in my app, and I think what you propose would work okay there. I'd have to move things around, but I suspect the result might turn out to be simpler in the end -- it would definitely remove a pushUrl
call in my code that just gets turned right around into another RouterMsg
AFAICT.
I do still need to see the external URLs in my app, and I think the way you've done that with onExternalUrlRequest
also seems right.
If it's okay, I'll take some time to rework my app to use this, and confirm my intuition. If it turns out the way I expect, I'll add your change to this PR!
Just an update: I still think this will work, but it needs an important tweak. As you've implemented it, there's nothing that causes the Browser.Navigation.pushUrl
call that's needed to actually update the URL. So we need to account for that. I think the approach I've taken in https://github.com/jerith666/elm-route-url/commit/32a3a0c552426cfa0a7820ec71eda62a21a59d1a will do the trick. But it's tangled up with some other unrelated changes I'd been working on, so I need to take some time to disentangle it for inclusion here.
Finally got some time to update the examples & README, and squash down to a single commit with a sensible message. Please take a look and let me know if I've missed anything!
I think you got everything in your well-crafted commit message. Thanks for that.
Also, just want to note that I appreciate the --debug
being used in the examples.
Browser.application
and using newelm/url
core APIRouteHash
RouteUrl.Builder
fixes #37.
adapt to the new Browser.application API in the following ways:
mirror the two-phase handling of URL changes in Browser.application's 'onUrlRequest' and 'onUrlChange' by bifurcating the RouterMsg variant of WrappedMsg into RouterMsgOnUrlChange and RouterMsgOnUrlRequestInternal variants.
add a slot to RouterModel to store the new Browser.Navigation.Key, and in the update function, use it to invoke Browser.Navigation.pushUrl in response to a urlRequestInternal.
create a new 'onExternalUrlRequest' function for the user to implement, since RouteUrl can handle internal requests for the user, but can't do anything sensible with external requests (as suggested by @basti1302).
eliminate the distinction between App and AppWithFlags, and all related duplication, as there is no variant of the new Browser.application that doesn't support flags.
make UrlChange more strongly typed, mirroring the structure of the Url.Url record type from elm/url, and rework the way UrlChanges are converted to Cmds with a new 'apply : Url -> UrlChange -> Url' function.
update all examples to work with the new API and 0.19 generally
include work-arounds for a couple of elm/url bugs (https://github.com/elm/url/issues/37 and https://github.com/elm/url/issues/17)
store the base path in ExampleViewer.Model to illustrate absolute path requirement of UrlChange
build the examples with '--debug' so users can get an idea for how they work under the hood
update README
remove the RouteUrl.Builder module and the use of the sporto/erl package, as this functionality is now largely provided by elm/url.
remove the older RouteHash module, as it was only present to ease the transition from elm-route-hash to elm-route-url. also remove example code illustrating its use.