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

Fable 4.0 breaks parseParams #25

Closed
forki opened this issue Mar 20, 2023 · 6 comments · Fixed by #28
Closed

Fable 4.0 breaks parseParams #25

forki opened this issue Mar 20, 2023 · 6 comments · Fixed by #28

Comments

@forki
Copy link

forki commented Mar 20, 2023

Before the Fable 4.0 update we did:

let urlParser location = parsePath pageParser location

but now we have to do

let urlParser (location:Browser.Types.Location) =
    parse pageParser location.pathname (parseParams (location.search.Replace("?","")))

It looks like the ? is no longer handled automatically.

/cc @et1975 @alfonsogarciacaro @MangelMaxime @tforkmann

@alfonsogarciacaro
Copy link

That's strange. Did you only update Fable or also Elmish? Would it be possible to print the generated code for Fable 3 and Fable 4 for urlParser?

@forki
Copy link
Author

forki commented Mar 20, 2023

for the record: I don't think it's a fable compiler bug.

@et1975
Copy link
Member

et1975 commented Mar 20, 2023

This library doesn't work on Browser.Types.Location because it was meant to be cross-platform from the get-go. The original version baked into Fable.Elmish.Browser prior to the split did have that dependency though, so maybe that's the change you're observing.

@mlaily
Copy link

mlaily commented Oct 14, 2023

Hello!

I don't have any idea about the history of these projects, but I confirm there is currently an incompatibility between them:

parsePath and parseHash from Fable.Elmish.Browser (https://github.com/elmish/browser/blob/v4.x/src/parser.fs#L16-L33) do not remove the "?" from the query param string, and pass it directly to parseParams from Fable.Elmish.UrlParser (https://github.com/elmish/urlParser/blob/master/src/parser.fs#L356)

The issue is that this parseParams expects the string to not contain the initial "?" (as hinted by the fact the parseUrl from Fable.Elmish.UrlParser does remove it before using parseParams https://github.com/elmish/urlParser/blob/master/src/parser.fs#L368-L375)

I'm not sure what's the best approach to fix it, but I see two options:

  • Fable.Elmish.UrlParser.parseParams is modified to remove the initial "?" if present. This is a breaking change, so probably not the best option.
  • Fable.Elmish.Browser.parsePath and Fable.Elmish.Browser.parseHash are modified to use a new implementation of parseParams that knows to remove the initial "?". (This new Fable.Elmish.Browser.parseParams function could either be public or private, but since it lives in the same namespace as the original one from the other project, it could lead to more confusion if it's public...)

This reimplementation of parseParams in Fable.Elmish.Browser could take inspiration from Feliz.Router and use the native URLSearchParams query param parser (that expects an initial "?", so it would fit naturally) https://github.com/Zaid-Ajaj/Feliz.Router/blob/master/src/Router.fs#L141-L142


Based on the above, here is what I came up with (just for parseHash, but it should work the same for parsePath):

    type private IUrlSearchParameters =
        abstract entries : unit -> seq<string array>

    [<Emit("new URLSearchParams($0)")>]
    let private createUrlSearchParams (_queryString: string) : IUrlSearchParameters = jsNative

    let parseHash (parser: Parser<_,_>) (location: Browser.Types.Location) =
        let parseParams queryString =
            try
                let urlParams = createUrlSearchParams queryString
                Map [ for entry in urlParams.entries() -> entry[0], entry[1] ]
            with
            | _ -> Map.empty

        let hash, search =
            let hash =
                if location.hash.Length > 1 then location.hash.Substring(1)
                else ""
            if hash.Contains("?") then
                let justHash = hash.Substring(0, hash.IndexOf("?"))
                justHash, hash.Substring(justHash.Length)
            else
                hash, "?"

        parse parser hash (parseParams search)

It seems to work well as a workaround too, until this is fixed.

@et1975
Copy link
Member

et1975 commented Oct 17, 2023

Released fix in 1.0.2, please give it a spin.

@mlaily
Copy link

mlaily commented Oct 18, 2023

Released fix in 1.0.2, please give it a spin.

What about adding unit tests instead? :)

elmish/browser#69

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.

4 participants