rgrempel / elm-route-url

Router for single-page-apps in Elm
http://package.elm-lang.org/packages/rgrempel/elm-route-url/latest
MIT License
196 stars 16 forks source link

Upgrade to Elm 0.19 #37

Open rgrempel opened 6 years ago

rgrempel commented 6 years ago

I'll have to play with Elm 0.19 a bit to see what makes sense.

Elm's own theory about how to handle URLs has changed at least somewhat between 0.18 and 0.19. So, the question will be how the different approach used by this package fits in.

jerith666 commented 5 years ago

I've pondered for a while by reading docs, and convinced myself that there's still a need for this. So, I've started poking at it to see if the compiler agrees with me. :) See https://github.com/jerith666/elm-route-url/tree/update-019 for my fiddlings so far.

rgrempel commented 5 years ago

Thanks — I will take a look as soon as I can.

jerith666 commented 5 years ago

To be clear, it doesn't compile yet ... but my time comes in fits and spurts, so wanted to throw it out there in case it saves someone else a bit of time before I can pick it back up again. :)

jerith666 commented 5 years ago

Okay, pushed a few more commits. There are two compilation errors now, both require some thought.

in update, we now have reportedUrl = fromString urlChange.url. But unlike the old Erl library, which somehow had a parse : String -> Url function, the new core elm/url function is fromString : String -> Maybe Url -- it might not parse. So, what do we do in that case? (What did Erl do?)

My temptation is to push this problem outside of elm-route-url entirely, and make UrlChange's .url be of type Url rather than String. But the current API lets you change just the query or hash more easily, so maybe we need something to support that? Like

type alias UrlChangeMetadata =
    { entry : HistoryEntry
    , key : Key
    }

type UrlChange
    = NewPath
        UrlChangeMetadata
        { path : String
        , query : Maybe String
        , fragment : Maybe String
        }
    | NewQuery
        UrlChangeMetadata
        { query : String
        , fragment : Maybe String
        }
    | NewFragment UrlChangeMetadata String

Second, we don't have anything obvious to use for the onUrlRequest value in the record passed to Browser.application. I admit I haven't thought much at all about this one yet, wanted to get past the first issue I listed before contemplating this one too deeply.

jerith666 commented 5 years ago

Carried through the above-proposed UrlChange refactor, feels pretty reasonable, but we'll see.

Decided to just require the client code to supply an onUrlRequest function for us to delegate to, that also seems fairly reasonable, but again, we'll see once I get to some actual client code. :)

Down to no compilation errors in RouteUrl.elm, but still some in RouteUrl.Builder and RouteHash.

jerith666 commented 5 years ago

I've completed updating my photo album app to use this, and it both works okay and feels okay in the code. Commits on my project that show the changes include:

https://github.com/jerith666/elbum/commit/ce80ac05010181596f05cd911814e09ca43660db s/Location/Url/ in navToMsg https://github.com/jerith666/elbum/commit/13b395b1c7a15573afdc5720f5290056c33f0fbf use Maybe.withDefault for Url fields https://github.com/jerith666/elbum/commit/f064b5bbf9404c23b0a9995449e5472b0d748c1f?w=1 query and hash prefixes (? and #) are now included automatically https://github.com/jerith666/elbum/commit/b59f0e3229eaeb35b407b442523d89fa7b8fe412#diff-ca49af1e1ae6cd714ae3d1361c1b9d4eR773 add key field https://github.com/jerith666/elbum/commit/7a8d0e1428ffadc397d1c44941e228e374cf0937 only use NewQuery when query is nonempty

We'll need to update docs and examples before we have something releasable ... what do you think, is this the right direction?

ljnicol commented 5 years ago

Is there any further updates on this? I'd like to help push this through to 0.19 if you still think its worthwhile.

We use elm-route-url extensively in our application and its holding us back from upgrading to 0.19.

rgrempel commented 5 years ago

Ah, yes, I haven't taken a close look at this yet -- partly because I haven't had occasion to update the apps I'm working on to Elm 0.19 yet.

I'll make some time for this in the next few days!

jerith666 commented 5 years ago

If it'd be easier to have a PR open with my changes so far, just LMK!

ljnicol commented 5 years ago

@jerith666 That would be useful - were there many changes required to move to Elm 0.19?

jerith666 commented 5 years ago

@ljnicol Done, #38. I made a fair amount of changes, yes. And there is still work to be done, see the checklist in that PR.