funcool / bide

A simple routing library for ClojureScript
BSD 2-Clause "Simplified" License
132 stars 20 forks source link

Allow to pass customized instance of `goog.history.Html5History` to `… #9

Closed mt0erfztxt closed 7 years ago

mt0erfztxt commented 7 years ago

Hi and thanks for the lib.

Today I ran into something unexpected - after each call to navigate! query string in browser is duplicated. When searching the cause I found same issue for other routing lib and solution here. The solution was to use custom token transformer in goog.history.Html5History instance. Reading comments for start! helper I see "If you want use the html5 history instance providing a factory function that returns the Html5History object instance", but reading code for it I can't understand the way to provide such function! So, I fork the repo and add a little code to it. Now, passing manually created instance of Html5History I get rid of that issue - no more duplicates :)

Note: I don't understand how to run tests, lein test says "Ran 0 tests containing 0 assertions".

niwinz commented 7 years ago

Thanks you very much @mt0erfztxt

You are right, the docstring was wrong. However, I thinking that providing a instance of html5history is not a good approach, just because the start function executes side-effect operations on instance, that is not expected. I think that a better approach would be allow passing a factory function that is responsible of creating a new fresh instance of html5 history.

What do you think?

mt0erfztxt commented 7 years ago

As a developer I have little experience, so I thought a lot about what you said, and, most likely, you are right - we must not do any side-effects on history instance that was provided or must separately warn in docs about that side-effects. I think that using factory function here is a right way and, if I understood you correctly, the change is as simple as

...
(let [html5history (if (fn? html5history) (html5history) (Html5History.)
      history
      (if html5?
        (doto html5history (.setPathPrefix "") (.setUseFragment false) (.setEnabled true))
        (doto html5history (.setUseFragment true) (.setEnabled true)))
       initial-token (-initial-token history)
       initial-loc (-match initial-token)
       lkey (e/listen history EventType.NAVIGATE -on-navigate)]
...

or you mean something totally different?

niwinz commented 7 years ago

That's it!

mt0erfztxt commented 7 years ago

Done.