@richiardiandrea it really just doesn't seem that important

there's been countless weird stuff like this and we haven't noted this before

I don't see a compelling reason to start now

I also haven't seen people clamoring about this oddity in any of the channels

@dnolen oh just sounded like a sound thing to do for saving time for other folks

so I don't know what time saving this would offer

well, it would have saved my time for sure 😉

right but you have to account for other people's time who are complaining

I haven't seen much of it

yeah I see your point and the decision is of course yours, but the work has been done so I guess it is just a matter of deciding if it's work pushing a button...I would do it...

but then other people want to mark such things

so I don't see a need to encourage that

this one just seems too edge-casey sorry

kk just as a data point, one of the most used libs for reporting errors humane-test-output, was affected

having said that, it does very edge-casey things 😉

which lib?

sorry having some context helps me evaluate and Idon't remember anymore what this is aobut

also the PR is a context-less info hole

just saying

kk

I'm happy to have a different opinion if it's possible for me to evaluate your thought process

so...first of all it is not a big deal...but something that I have just been thinking a bit when things broke under my feet 😄

so there is this bindings

and before it was doing X, now it is doing what Clojure is doing

it's been like this forever and all that...

should it still be considered a breaking change and therefore signaled in the release notes?

your answer above suggests you don't think so, but gut feeling was saying that it should have

but I mean affecting a lib adds more context about impact

i.e. sometimes something we do will break Reagent

oh ok

and that's a showstopper

gotcha

but you have to say that

well the issue is this: https://github.com/pjstadig/humane-test-output/issues/37

why am I going to look there to accept a PR to ClojureScript docs?

it's not going to happen

please put all info in the PR

ok...

https://clojars.org/pjstadig/humane-test-output

if you just point out stuff like this, very different assessment

added more info in the PR...

than just seeing it as abstract bug

and I know you mentioned it before here

gotcha, I have to say that I opened the PR in anger 😉

but Slack is transient

no problem, I can repeat things if it's worth it

and my brain isn't that big 🙂

thanks!

you might argue that there is some technical debt to cleanup in that lib....you are actually probably right, but I am just the messenger 😄

trying to wrap up the next release, mostly to line up with spec and 1.10 stuff

@mfikes what's the status of https://dev.clojure.org/jira/browse/CLJS-2945 given CLJS-2913?

I’ll take a look in a bit and let you know

cool, thanks!

@dnolen I assigned this one to you: https://dev.clojure.org/jira/browse/CLJS-2964 because of the dependency on https://dev.clojure.org/jira/browse/CLJS-2955. without that, the self-host test fails.

@borkdude I unassigned 2955, doesn't apply anymore

maybe assign @mfikes since he wrote the patch?

anyway, now he probably saw it 🙂

so I was looking at CLJS-2999 yesterday while doing CLJS-3010 and was wondering if it was worth addressing the fact that the protocol namespaces are different between CLJ and CLJS

ah, that datafy is in core and not in it's own namespace?

yeah

I don't think we can make that work

the protocol method in CLJ is clojure.core.protocols/datafy

oh

in Clojure is datafy completely outside of core?

yes or no?

if no, then you cannot make it work

you can't have some.core.ns -> core

it’s clojure.datafy + protocols in clojure.core.protocols

defprotocol Datafiable is in clojure.core.protocols, and then the public implementation is in clojure.datafy

then yes, we should rearrange

cljs.datafy + cljs.core.protocols

which would aid portability

due to auto-aliasing of clojure nses

OK, great. shall I add those as notes to CLJS-2999?

oh rewinding

what's the problem here?

we have clojure.datafy already in ClojureScript

so the only thing that really needs to happen is cljs.core.protocols?

I suppose that could even be clojure.core.protocols

two things: - enable Datafiable to be extended via metadata - move Datafiable to match same namespace as CLJ so that metadata protocol extensions are portable for it

ah I see - got it - yes let's update to 2999 to reflect this, and I'm ok with it

we probably want to do the same for Navigable

yep

Sorry for the delay; I see above I have 2 things int the queue. Will try to get to them over the ext hour or two

@dnolen For https://dev.clojure.org/jira/browse/CLJS-2945 the issue still exists, with the root cause likely being that we’d need to add the CLJS-2913 improvements to the runtime side of things. I’ve been porting ex-triage / ex-str to ClojureScript, so I’ll see if I can cobble together an experimental patch for CLJS-2945. I suspect it would need some baking time before we are happy with it, but I’ll at least make a stab at it.

do we currently extend any protocols to JS errors?

looking for examples

Not that I’m aware of

mkay. still working with datafy, I’ve shuffled the namespace’s around to match CLJ and enabled extension via metadata. Still, there’s some differences: - if the datafied value differs from the value passed into datafy, clojure.datafy/datafy adds a ::class metadata with a symbol of the original value’s .className. I’m not sure how best to support this for all environments and through advanced compilation. - several default implementations of datafy, specifically: - Throwable - CLJS equivalent would be js/Error, although I’m not sure that covers all cases - clojure.lang.Namespace - no CLJS equivalent AFAIK - java.lang.Class - I’m not sure if this is possible, since JS “classes” are constructor functions

Java class stuff is meaningless

so let's put that to one side

what is using the class intended to do?

that's all that matters

https://dev.clojure.org/jira/browse/CLJS-2945 now has an experimental patch that shows a potential avenue for essentially pushing the improvements to exception handling down into the runtime for what I think is :clojure.error/phase :execution

(For self-hosted, you might have all phases in ClojureScript, but perhaps the truth is for JVM ClojureScript, you end up with the typical Clojure / JavaScript split you might expect.)

It worked, producing the following, but, there’s honestly a lot of work that would need to be done before we are happy with it:

cljs.user=> (foo "A")
Execution error - invalid arguments to cljs.user/foo at (<cljs repl>:1).
"A" - failed: int? at: [:n]

cljs.user=> (ffirst 1)
Execution error (Error) at (<cljs repl>:1).
1 is not ISeqable

@mfikes hrm it seems no phase except for :execution should be in the runtime no?

all other phases are in the analyzer, compiler or repl

Right. The only reason to include the other phases is if we wanted self-hosted users to be able to use them. But, to be honest, they could all be deleted from there.

Converse is true for the Clojure part, it could drop the :execution case

(I think)

actually that true

The source of this patch was some work I was doing for Planck, where all the phases exist.

sorry spaced on self-hosted, so yeah that's ok

but we do need this stuff at the compile time level right?

Yeah, for self-hosted, the code could be there dormant, but usable by REPLs like Planck or Lumo if they wish

We might be able to drop this case: https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/repl.cljc#L893

oh I see, already done

@mfikes so what's missing here really then? I felt you were implying that

Well, you raised a good point, we have the case above that may not be used in Clojure, and the converse of all of those in ClojureScript. (An interesting aside is that repl.cljc could in the future be used for both Clojure and ClojureScript, but for now it is still really Clojure.) But, TL;DR, the extra cases on either side hurt nothing, and on the ClojureScript side, the extra cases could be used for self-hosted. One thing that is missing from that patch is that it only revises the Node stuff, and didn’t do anything for browser, etc. It also interestingly might mess with current code that expect a parseable stacktrace in that string. But, having said that, pst still seems to work properly. So, in short, I bet this kind of patch might be in the direction we want. I just slapped it together so quickly, I’m nervous that it is solid yet.

@mfikes I see, but can't we just wire extract-canonical-stacktrace to this existing runtime stracktrace stuff?

we made that stuff .cljc I thought? (or duped it at least)

stacktrace.cljc

(or was there something challenging you saw there that I missed?)

also happy to do this in two phases, if just a matter of time

Oh, yeah, the extract-canonical-stacktrace could be wired up. (It may not even really be needed—that was from WIP where I was trying to copy Clojure’s Throwable->map. But yeah, in general, I don’t see any huge challenges. Just some good old fashioned work to bring that patch up to a higher level of quality. Alternatively, a derivative of that patch could be landed and further refined over time

merge this one and I can do that wiring

Ok. Will do.

thanks!

Just running tests before merging...

@lilactown patch looks ok but weren't there tests for datafy? Pretty sure there were so I don't see how they could work now?

(also nav)

hehe, yeah. Just realized I broke the tests. Fixing now

ok cool 🙂

Patch merged / landed for https://dev.clojure.org/jira/browse/CLJS-2945 and captured follow-on tweak work needed for other shipping non-Node REPLs: https://dev.clojure.org/jira/browse/CLJS-3011

Set CLJS-3011 to blocker so we don't forget about it

thanks!