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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jerith666
Copy link

@jerith666 jerith666 commented Dec 19, 2018

  • get main body of code compiling with 0.19, compatible with Browser.application and using new elm/url core API
  • retire ancillary modules that are no longer useful
    • RouteHash
    • RouteUrl.Builder
  • update docs
  • update examples
  • 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

  • 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 jerith666 mentioned this pull request Dec 19, 2018
@lenards
Copy link

lenards commented Jan 16, 2019

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

@jerith666
Copy link
Author

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

@lenards
Copy link

lenards commented Jan 17, 2019 via email

@lenards
Copy link

lenards commented Jan 31, 2019 via email

@basti1302
Copy link

basti1302 commented Jul 4, 2020

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: basti1302@aeef0e7

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.

@jerith666
Copy link
Author

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
Copy link
Author

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 jerith666@32a3a0c 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 jerith666 changed the title WIP: Update to 0.19 Update from Elm 0.18's Navigation.program to Elm 0.19's Browser.application Jul 28, 2020
@jerith666
Copy link
Author

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!

…cation

* 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
    (elm/url#37 and
    elm/url#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.
@lenards
Copy link

lenards commented Jul 28, 2020

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.

, button [ onClick Increment ] [ text "+" ]
]


countStyle : Attribute any
countStyle : List (Attribute any)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is in the original, but I really like the use of any as the name of a type variable.

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

Successfully merging this pull request may close these issues.

Upgrade to Elm 0.19
3 participants