elm-route-url icon indicating copy to clipboard operation
elm-route-url copied to clipboard

Update from Elm 0.18's Navigation.program to Elm 0.19's Browser.application

Open jerith666 opened this issue 6 years ago • 9 comments

  • [x] get main body of code compiling with 0.19, compatible with Browser.application and using new elm/url core API
  • [x] retire ancillary modules that are no longer useful
    • [x] RouteHash
    • [x] RouteUrl.Builder
  • [x] update docs
  • [x] update examples
  • [x] squash commits down to a sensible history

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 references to complementary packages that aren't compatible with 0.19 (which is all of them)
  • 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.

jerith666 avatar Dec 19 '18 01:12 jerith666

Hey @jerith666, if you want any help with this, I would like to see this upgraded.

lenards avatar Jan 16 '19 02:01 lenards

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.

jerith666 avatar Jan 17 '19 04:01 jerith666

Okay.

On Wed, Jan 16, 2019 at 9:13 PM Matt McHenry [email protected] 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 .

lenards avatar Jan 17 '19 04:01 lenards

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 [email protected] wrote:

Okay.

On Wed, Jan 16, 2019 at 9:13 PM Matt McHenry [email protected] 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 .

lenards avatar Jan 31 '19 19:01 lenards

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 UrlRequests 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.

basti1302 avatar Jul 04 '20 17:07 basti1302

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!

jerith666 avatar Jul 10 '20 21:07 jerith666

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.

jerith666 avatar Jul 15 '20 00:07 jerith666

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!

jerith666 avatar Jul 28 '20 02:07 jerith666

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.

lenards avatar Jul 28 '20 15:07 lenards