Fork me on GitHub
Slack Archive

This page is not created by, affiliated with, or supported by Slack Technologies, Inc.

2018-12-04

Channels

#cljs-dev
<
2018-12-04
>
thheller13:12:15

@dnolen any objections before I attempt to fix this? https://dev.clojure.org/jira/browse/CLJS-3003

dnolen15:12:46

@thheller I'm a bit confused by the report since I thought we already did that, in fact your advanced snippet at the bottom does what you say no? Dispatch on arity and invoke the right methods?

thheller15:12:57

@dnolen no. it just happens to delegate to cljs.core/get in both cases. in the keyword case thats all the impl does so it looks like it is doing the right thing. if the actual impl is longer the full code is repeated.

thheller15:12:52

(deftype Repeated [a]
  IFn
  (-invoke [_]
    (js/console.log "repeated")
    a))

thheller15:12:11

demo.browser.Repeated.prototype.call = (function (self__){
var self__ = this;
var self____$1 = this;
var _ = self____$1;
console.log("repeated");

return self__.a;
});


demo.browser.Repeated.prototype.cljs$core$IFn$_invoke$arity$0 = (function (){
var self__ = this;
var _ = this;
console.log("repeated");

return self__.a;
});

dnolen15:12:34

ok, sounds good to me

👍 4
thheller20:12:27

@mfikes I can't figure out why the patch for https://dev.clojure.org/jira/browse/CLJS-3003 fails the self-host tests. maybe you have an idea? can't figure it out. the generated code seems fine for non self-host.

mfikes20:12:45

@thheller I’ll take a look at it when I get a chance (maybe later on tonight)

👍 4
thheller23:12:07

found the problem. just bad logic on my part.

mfikes23:12:33

Cool. The ->> in the patch needs to be qualified as core/->> FWIW.

mfikes23:12:53

With that change, I see an error in test-cljs-2133 and a failure in test-cljs-2741 when running script/test-self-host. Perhaps you've sorted that?

thheller23:12:20

new patch doesn't use ->> anymore. but the issue was that it always started with arity 0..n even if only n was actually declared

mfikes23:12:30

Ahh, cool.

thheller23:12:09

hmm I thought variadic protocols were not supported?

mfikes23:12:11

I put that new patch through CI / Canary. By the way, frequently updated patches are attached instead of replacing the originals.

mfikes23:12:23

Right, that's my memory as well.

thheller23:12:41

I typically delete patches to avoid confusion which the correct one is but I can attach as instead if preferred

mfikes23:12:45

Cool. We may have clarfication on what to do via https://github.com/clojure/clojurescript-site/pull/266 Or, we can switch to using GitHub.

👍 4
thheller23:12:27

that was the previous failure

mfikes23:12:34

Ahh, fuck.

mfikes23:12:40

Let's switch to using PRs. 🙂

thheller23:12:32

@dnolen my change in CLJS-3003 breaks https://dev.clojure.org/jira/browse/CLJS-2133. should this still actually be supported given that it officially isn't supported?

mfikes23:12:16

My vague memory on that one was that we reverted it so as to avoid lots of downstream failures by Reagent. Hopefully in the intervening year+ Reagent fixed it...

thheller23:12:30

yeah its fixed in reagent

mfikes23:12:57

Ahh cool, the diagnostic has been in place since 1.9.660

mfikes23:12:19

(CLJS-2134)

thheller23:12:05

I guess we can treat this as a deprecation period and finally remove it? seems weird to keep it around forever. I can however adjust the patch if requested.

mfikes23:12:30

I wonder if David is thinking of releasing soon to match Clojure 1.10. If so, perhaps CLJS-3003 could be applied soon after.

mfikes23:12:26

Or maybe it’s been long enough...

thheller23:12:15

yeah no rush on that one.

thheller23:12:11

@mfikes do you need to trigger your CI stuff manually? If so please do for https://dev.clojure.org/jira/browse/CLJS-3002