Open realgenekim opened 4 years ago
r0man
commented
4 years ago @realgenekim Thanks :) I somehow missed issue #124. Yes, I also think this could be due to shadow-cljs using a newer version of the Google Closure lib. You could try the solution proposed in #124 and if this is also working with stock ClojureScript we can add this to cljs-http. PR welcome!
realgenekim
commented
4 years ago Roger that! I'll take a stab at it in the next couple of days —
PS: Is it common to have Google Closure library API changes like this, where it breaks existing callers?
realgenekim
commented
4 years ago @r0man Okay, I have a version of cljs-http.core/jsonp that works with the versions of Google Closure compiler that shadow-cljs uses... 🎉🎉🎉 I'll post a link to the diffs tomorrow.
But it just hit me — seems like there should be a code path for the older versions of Google Closure goog.net, and one for the newer versions — how does one determine what version of the Closure compiler is being used? 🤔
Or to broaden the question, how do we tell which data structure to pass the jsonp function — a Uri or TrustedResourceUrl?
Thank you!
thheller
commented
4 years ago FWIW the "trusted" setup that parts of the closure-library use is purely for code-review purposes inside Google. They aren't actually enforced by the Compiler so you may "cheat" to get things to behave like you want.
I haven't looked into this issue in particular but it appears that it is just calling a cloneWithParams method in the uri you handed it. So in theory you can extend goog.Uri to have that method and then have that return something JsonP is happy with. How exactly that looks I don't know but I think it should work?
(set! js/goog.Uri.prototype.cloneWithParams
(fn [params]
(this-as goog-uri-instance
;; return whatever is needed?
)))@thheller What a clever idea. I'll have something in the next couple of days!
realgenekim
commented
4 years ago Hi, @thheller and @r0man — not sure how to proceed on this one... I have had new JSONP call code working locally since Feb 2, but just recently found a React component that obviates need for this code entirely. Which means I can delete my local version of cljs-http.
Here's the diff of what I have running — can you advise on what I can do to be most helpful, as JSONP definitely has a breaking change. Should I clean this up and put it into a pull request? (@thheller, I used a simpler method that I actually understood to create code that Google Closure would accept it.)
Diffs are below — I'll put together a pull request, too. (May take me a bit: I've never modified code in a library before. 😱. Be back in an hour? 🤞)
diff --git a/src/cljs/cljs_http/core.cljs b/src/cljs/cljs_http/core.cljs
index 314f620..a3bd5ef 100644
--- a/src/cljs/cljs_http/core.cljs
+++ b/src/cljs/cljs_http/core.cljs
@@ -114,6 +114,12 @@
(.abort xhr)))))
channel))
+(defn build-trusted-string [s]
+ ; this circumvents the enforcement of Const objects from non-string-literals
+ ; without this, Google Closure compiler will generate an error at compile time (i.e., shadow-cljs release mode)
+ ; https://github.com/google/closure-library/blob/master/closure/goog/string/const.js#L50
+ (Const. Const.GOOG_STRING_CONSTRUCTOR_TOKEN_PRIVATE_ s))
+
(defn jsonp
"Execute the JSONP request corresponding to the given Ring request
map and return a core.async channel."
@@ -122,7 +128,11 @@
:as request}]
(let [channel (async/chan)
;_ (println "*** jsonp: req: " request)
- trusted-url (TrustedResourceUrl/fromConstant (Const/from (:saved-url request)))
+ ;trusted-url (TrustedResourceUrl/fromConstant (Const/from (:saved-url request)))
+ trusted-url (TrustedResourceUrl/fromConstant
+ (build-trusted-string (:saved-url request)))
+ ;trusted-url (TrustedResourceUrl/fromConstant
+ ; (Const/from "https://publish.twitter.com/oembed?url=https://twitter.com/realgenekim/status/1223341376475222020&omit_script=true"))
;url (util/build-url new-req)
jsonp (Jsonp. trusted-url callback-name)]
(.setRequestTimeout jsonp timeout)
@@ -146,14 +156,16 @@
(.cancel jsonp req)))))
channel))
@thheller Alas, I think this might be beyond my skills. (I'm even having trouble getting a REPL session open — I'm guessing I need to create a new lein project, with checkouts configured? In the meantime, I'm just trying to run the tests via doo.)
(I honestly have no idea how to caller determines which input type is needed, as dictated by which version of the library is running.)
lein doo chrome-headless none?Here's a link to my diffs. I'll take any advice or suggestions, but at this point, consider me stuck. Thank you!
https://github.com/r0man/cljs-http/compare/master...realgenekim:master
thheller
commented
4 years ago Sorry can't help much. The whole "Trusted" setup inside the closure library code is very useful but it is very invasive and sort of an all or nothing proposition. If you don't do it properly everywhere you might as well not do it at all. Just hacking arround it is definitely not the right approach. So I'd probably just write the 20 lines of code to get Jsonp working myself in CLJS without those checks and stop using goog.net.Jsonp completely. You can try goog.html.legacyconversions.trustedResourceUrlFromString for the conversion aspects but that doesn't solve the issues if you want this to work with older CLJS versions.
This should now affect all newer CLJS releases since that is now using the same closure-library version as shadow-cljs.
@r0man — thank you for this wonderful library. I've been using it for 2+ years. I recently switched an app from figwheel to shadow-cljs, and now
jsonpfails. Maybe because it uses a newer version of the Google Closure library?I thought this might have had to do with the issue #124, which appears to be because of an interface change: " goog.net.Jsonp expect uri to be goog.html.TrustedResourceUrl (string of goog.Uri were valid before)".
Exact error from a browser REPL below — any advice would be much appreciated. Thanks in advance!!!