Fork me on GitHub
#cljs-dev
<
2016-08-14
>
thheller08:08:37

@dnolen you skipped over issue B+C on http://dev.clojure.org/jira/browse/CLJS-1733, should I report the as new issues or do you not agree that they should at least warn?

mfikes13:08:12

The only thing I can add to the odd KeySeq issue above is that I also saw the underlying mseq containing nil values in addition to map entry pairs. I can only reproduce this downstream so I don't want to add further noise here (it could just be a downstream-specific issue).

dnolen13:08:12

@thheller separate issues please, combining issue like that not desirable

dnolen13:08:25

@mfikes CLJS-1744 is same as the previous one but for create-array-node-seq, I moved the rest logic where it belongs and left the ctor fns alone

mfikes13:08:32

@dnolen: I can confirm that with those revisions Planck no longer exhibits the issue.

anmonteiro14:08:36

@dnolen: I wonder if this has landed with recent enhancements, or if I’m not understanding what the issue is http://dev.clojure.org/jira/browse/CLJS-1029

dnolen14:08:23

@anmonteiro: not addressed by the recent enhancements - but also probably not a good idea either. Will close 🙂

anmonteiro15:08:57

@mfikes: just attached a patch to http://dev.clojure.org/jira/browse/CLJS-1274 I don’t foresee any self-host problems, and both self-host and self-parity tests are passing, could you confirm when you got some time?

mfikes15:08:28

@anmonteiro: Do you know precisely the shape of the macro that this is intended to support? This is perhaps close but not really what the ticket is about:

(defmacro defvar [name value]
  `(def ~name ~value))

anmonteiro15:08:46

@mfikes: I think it’s meant to support things like this:

(defmacro deffoo [val]
  `(def foo ~val))

anmonteiro15:08:39

where previously you had to do:

(defmacro deffoo [val]
  `(def ~'foo ~val))

mfikes15:08:16

@anmonteiro: Ahh. OK. That fundamentally won’t work as is in bootstrap because (deffoo 12) expands to (def foo.core$macros/foo 12)

mfikes15:08:46

Perhaps that’s a general problem with bootstrap, not with your patch.

anmonteiro15:08:32

@mfikes: right but then it should probably throw with my patch?

anmonteiro15:08:50

because foo.core$macros wouldn’t be equal to foo.core

anmonteiro15:08:04

so it wouldn’t allow to define that ns-qualified sym

anmonteiro15:08:16

which is the old behavior

mfikes15:08:18

Right. Your patch behaves that way. :)

anmonteiro15:08:04

@mfikes: (def cljs.user/foo 42) should however work in bootstrap

anmonteiro15:08:13

if you’re in the cljs-user NS

anmonteiro15:08:20

but that’s just not useful for anything at all

mfikes15:08:42

One minor side comment on your patch is that the analysis error could perhaps be relaxed. Right now it sounds unconditional: Can't def ns-qualified name at line 1

mfikes15:08:37

Yep… just trying to do the due diligence to see if the patch meets the goals of the ticket, even under bootstrap. (I suppose it can’t.)

anmonteiro15:08:24

@mfikes: hrm, yeah, perhaps the error message should be changed

anmonteiro15:08:48

Clojure’s is not really helpful for this situation:

Can't refer to qualified var that doesn't exist

anmonteiro15:08:25

maybe Can’t def ns-qualified name in other namespace

mfikes16:08:44

Yeah, something along those lines of relaxation was what I was thinking too :)

mfikes16:08:39

Perhaps the other namespace is available as a parameter to go in the message text?

mfikes16:08:42

(Not a serious thought, but knowing we have a separate macros namespace, it it an interesting idea if there were variations on syntax quote to control whether things expanded to the macro or runtime namespace.)

anmonteiro16:08:50

we do have the other ns param

anmonteiro16:08:02

probably worth putting that into the error message too

anmonteiro16:08:17

I’ll amend the patch to include that

anmonteiro16:08:52

cljs.user=> (def foo.core/foo 43)
Can't def ns-qualified name in namespace foo.core

mfikes16:08:46

That will help if someone bumps into this in bootstrap, as it would effectively end up emitting a diagnostic that reads Can't def ns-qualified name in namespace foo.core$macros

anmonteiro16:08:05

Attaching the amended patch just now

anmonteiro16:08:21

Thanks for the feedback, as usual!

mfikes16:08:13

@anmonteiro: Your second patch is identical to the first.

anmonteiro16:08:28

@mfikes: ooops, I forgot to commit

anmonteiro16:08:14

@dnolen: added a comment to this one on why it should probably be closed: http://dev.clojure.org/jira/browse/CLJS-1242

anmonteiro16:08:26

regexp? already exists and serves the same purpose, I think

anmonteiro17:08:34

@dnolen: I wonder if this one is in fact a bug or not. http://dev.clojure.org/jira/browse/CLJS-1623 It would seem to me that using with-redefs under :static-fns is a no go

anmonteiro17:08:00

so the error is kind of expected

dnolen17:08:16

hrm right yeah, in anycase not a priority

dnolen17:08:02

I think vars marked dynamic don’t get optimized right? that’s probably the only thing I’m interested in - a way to work around it

dnolen17:08:25

that is with-redefs on vars declared dynamic should work

anmonteiro17:08:45

hrm let me check that

anmonteiro17:08:41

I understand it’s not a priority, just doing some basic screening and questioning myself when something obvious jumps to sight

anmonteiro17:08:59

@dnolen: right, :dynamic vars won’t get optimized, so with-redefs works under :static-fns true Added a comment to the issue that demonstrates this: http://dev.clojure.org/jira/browse/CLJS-1623?focusedCommentId=43541#comment-43541

richiardiandrea17:08:09

so I think the problem with replumb is that before we had the file js/compiled/out/cljs/string.cljs

richiardiandrea17:08:37

that now is not generated anymore from what I see...we have js/compiled/out/clojure/string.cljs

anmonteiro17:08:04

self-host passes with current master and your test suggestion

mfikes17:08:42

@anmonteiro: The setup down in script/test-self-parity employs the 3rd strategy in my comment in that ticket to make things work for certain namespaces. The fundamental problem is that self host can’t directly read from the filesystem in order to learn if implicit macro loading should be in effect. (But it is possible that some subtle things have changed since that ticket was authored.)

richiardiandrea18:08:09

Has this already been fixed in master? WARNING: Wrong number of args (5) passed to cljs.analyzer/check-use-macros-inferring-missing at line 479 dev-resources/private/node/js/compiled/out/cljs/js.cljs

richiardiandrea18:08:28

I am always late with my notifications 😄

dnolen18:08:07

@richiardiandrea: cljs.string isn’t and never was a thing

dnolen18:08:16

so no idea what’s going wrong for you

richiardiandrea18:08:52

yes sorry about that, I think I misinterpreted some error here, it looks the problem is elsewhere

dnolen18:08:22

@anmonteiro: closed the with-redefs ticket

mfikes18:08:16

@anmonteiro: I’ve confirmed your observation with respect to http://dev.clojure.org/jira/browse/CLJS-1657 and added a recommendation to the ticket.

anmonteiro18:08:14

happy to get some feedback on the docstring of the refer-clojure REPL special: http://dev.clojure.org/jira/browse/CLJS-1742

anmonteiro18:08:29

Used most of the wording in the doc for clojure.core/refer, adapted to the ClojureScript use case

mfikes19:08:10

@anmonteiro: I wonder if the “via inclusion” language is related to :only. (Just a guess… hmm…)

anmonteiro19:08:50

Ah right, I missed that one

anmonteiro19:08:29

@mfikes: does this sound good?

Filters can be used to select a subset, via exclusion, or to provide a mapping
  to a symbol different from the var's name, in order to prevent clashes.

anmonteiro19:08:58

or should I just eliminate the subset part altogether

mfikes19:08:59

I think, so. I’m still trying to understand what, for example, (refer-clojure :exclude [juxt]) really does…

anmonteiro19:08:53

theoretically, it unmaps cljs.core/juxt from the current NS, right?

anmonteiro19:08:45

not sure how it would do that in CLJS though

anmonteiro19:08:50

or how it really works

mfikes19:08:40

We are used to :refer-clojure :exclude in ns forms to effectively suppress warnings when you redefine core fns. But, right… at the REPL, it is not clear to me what this even means in Clojure.

mfikes19:08:01

It doesn’t appear to unmap FWIW

anmonteiro19:08:37

It doesn’t unmap, no, I’ve tried that before

mfikes19:08:45

When I wrote that JIRA I avoided writing the docstring myself because of this lack of understanding 🙂

anmonteiro19:08:04

also why I requested feedback on my patch

richiardiandrea19:08:19

so I understood the problem, it seems that now every time cljs resolves a clojure.string deps and tries to load it, the *load-fn* is called with cljs.string instead

richiardiandrea19:08:45

this happens for transitive js deps, for instance of goog/string/stringbuffer

richiardiandrea19:08:16

so before I was receiving clojure.string, now I receive cljs.string, which does not exists

dnolen19:08:17

@richiardiandrea: it tries clojure.string first

dnolen19:08:53

or it should, so that’s what you should be looking for if you’re trying to suss out a bug

richiardiandrea19:08:42

@dnolen: thanks for the confirm. if you say that, maybe there is something there, I will check the cljs source

dnolen19:08:08

@anmonteiro: @mfikes: :exclude just removes that name from being checked for existence in core

mfikes19:08:03

I found that Clojure’s refer-clojure actually does something close to its name, and wrote a ticket for ClojureScript: http://dev.clojure.org/jira/browse/CLJS-1745

dnolen19:08:48

@mfikes I don’t think the ticket warrants Major

mfikes19:08:02

I was thiking the same after the fact 🙂

dnolen19:08:12

it works well enough right now, but sure the behavior could use some finesse

mfikes19:08:05

Knocked it down to minor. In truth that’s the kind of ticket that could sit there a long time as noise 😞

richiardiandrea20:08:47

Sorry for bothering again on this, just a confirm, do i understand right in saying: first, clojure.string is sent to load-fn but then whatever the result of it (load or fail), cljs.string is tried. So before load-fn was receiving one call, now two.

richiardiandrea20:08:31

If the above is correct I need to keep track of the load result and avoid the second load here

mfikes20:08:47

@richiardiandrea: No, only if a load fails, and only if the initial segment of the namespace is clojure, does it try again replacing the initial segment with cljs.

richiardiandrea20:08:40

thanks, I am threading through js.cljs with a lot of debug-prn 😄

richiardiandrea20:08:03

yep, I put a debug-prn there to understand why the load fails and:

core.cljs:155 Loading result:  {:error #error {:message Could not parse ns form clojure.string, :data {:tag :cljs/analysis-error}, :cause #object[Error Error: No protocol method IDeref.-deref defined for type null: ]}}

mfikes20:08:30

In other words, if there was no error loading clojure.string continue on with load-deps on the (next deps), otherwise, if-let can convert to cljs-dep, try a require on it.

richiardiandrea20:08:36

would be good to add this log permanently

richiardiandrea20:08:58

thanks Mike, that put me on the right path

mfikes20:08:14

Ahh, yeah, the failure to load clojure.string is being discarded in this case...

richiardiandrea20:08:07

I will open an issue, it is a one liner patch...so that we at least see it in the logs

mfikes20:08:24

Yeah, and in your specific case, perhaps when processing (ns clojure.string… it fails to (:require [goog.string, etc. with the IDeref.-deref, it could be right around there.