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

Add custom TokenTransformer #20

Merged
merged 2 commits into from
Aug 20, 2018
Merged

Conversation

andsnpl
Copy link
Contributor

@andsnpl andsnpl commented Jul 31, 2018

After encountering #15 I did some debugging and it looks like this occurs because in non-fragment mode, Html5History implements a default token<->url transform that expects token to be derived from the pathname only. When receiving a new token through navigate! that does include query-string, it appends the existing query string, resulting in duplication. See this line in the implementation.

This adds an implementation of the interface defined for overriding that mapping, a public function to instantiate it, and a couple of sentences of documentation.

Tested this in a browser repl on my own machine, but did not write formal tests. I think it needs an integration test running in the browser (or to mock the window object?) Happy to do so, but don't know how that is commonly done in ClojureScript. Examples welcome!

@niwinz
Copy link
Member

niwinz commented Jul 31, 2018

Nice work, writing integration tests here is complicated, I think that the best (and quick way) is just release a snapshot and let the people try it on their projects.

I'll merge it ASAP.
Thanks

@andsnpl
Copy link
Contributor Author

andsnpl commented Jul 31, 2018

@niwinz Is that just a matter of changing the version to 1.6.1-SNAPSHOT in project.clj?

@andsnpl
Copy link
Contributor Author

andsnpl commented Aug 4, 2018

@niwinz bumping this. Are there any next steps from my end?

@niwinz niwinz merged commit 64ff2dd into funcool:master Aug 20, 2018
@niwinz
Copy link
Member

niwinz commented Aug 20, 2018

Pushed the snapshot to clojars.

@bensu
Copy link

bensu commented Oct 22, 2018

The fix worked for me: I no longer get accumulated query parameters as described in #15

The problem is that in

What I see:

  • my ClojureScript output directory hast the file bide/impl/TokenTransformer.js in it.
  • figwheel complains that it can't find

@bensu
Copy link

bensu commented Oct 22, 2018

The fix worked for me: I no longer get accumulated query parameters as described in #15

The new problem is that the current snapshot (bide-1.6.1-20180820.103728-1) doesn't compile:

What I see:

  • my ClojureScript output directory hast the file bide/impl/TokenTransformer.js in it (snake case name)

  • figwheel complains that it can't find it:

---  Could not Analyze  resources/public/js/web-out/bide/core.cljs  ----

  No such namespace: bide.impl.TokenTransformer, could not locate bide/impl/TokenTransformer.cljs, bide/impl/TokenTransformer.cljc, or JavaScript source providing "bide.impl.TokenTransformer"

----  Analysis Error : Please see resources/public/js/web-out/bide/core.cljs  ----
  • cljsbuild also can't find it:
Caused by: clojure.lang.ExceptionInfo: No such namespace: bide.impl.TokenTransformer, could not locate bide/impl/TokenTransformer.cljs, bide/impl/TokenTransformer.cljc, or JavaScript source providing "bide.impl.TokenTransformer" in file target/web/bide/core.cljs {:tag :cljs/analysis-error}
  • I noticed that in the PR the file is spelled in lowercase bide/impl/tokentransformer.js. When I add a copy of that file to my classpath, then figwheel and cljsbuild compile properly, and navigate! appends query when using html5 #15 is properly resolved.

  • the downloaded jar in ~/.m2 has a lowercase name too. For some reason the build tools are copying it as TokenTransformer.js or not finding it.

It might be a bug of the build tools but I could reproduce the problem and the fix (adding a lowercase copy in my classpath) in both cljsbuild and figwheel. It could also be a bug of the ClojureScript compiler.

@bensu
Copy link

bensu commented Oct 23, 2018

Finally, I tried adding bide/impl/TokenTransformer.js to my CircleCI repository and it successfully compiled, while adding bide/impl/tokentransformer.js didn't. I suspect it might be a matter of renaming the file to bide/impl/TokenTransformer.js

@niwinz
Copy link
Member

niwinz commented Oct 25, 2018

Renamed and pushed new snapshot to clojars.

@bensu
Copy link

bensu commented Oct 25, 2018

Thank you! The new snapshot version compiled without problems.

@niwinz
Copy link
Member

niwinz commented Oct 26, 2018

nice, i will release a new version today probably

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.

3 participants