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

Upgrade to Elm 0.19 #37

Open
rgrempel opened this issue Aug 25, 2018 · 11 comments · May be fixed by #38
Open

Upgrade to Elm 0.19 #37

rgrempel opened this issue Aug 25, 2018 · 11 comments · May be fixed by #38

Comments

@rgrempel
Copy link
Owner

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

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

rgrempel commented Oct 7, 2018

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

@jerith666
Copy link

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

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

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

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:

jerith666/elbum@ce80ac0 s/Location/Url/ in navToMsg
jerith666/elbum@13b395b use Maybe.withDefault for Url fields
jerith666/elbum@f064b5b?w=1 query and hash prefixes (? and #) are now included automatically
jerith666/elbum@b59f0e3#diff-ca49af1e1ae6cd714ae3d1361c1b9d4eR773 add key field
jerith666/elbum@7a8d0e1 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
Copy link

ljnicol commented Dec 4, 2018

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

rgrempel commented Dec 4, 2018

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

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

@ljnicol
Copy link

ljnicol commented Dec 19, 2018

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

@jerith666
Copy link

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

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 a pull request may close this issue.

3 participants