Fork me on GitHub
#cljs-dev
<
2018-12-14
>
dnolen00:12:05

@richiardiandrea it really just doesn't seem that important

dnolen00:12:14

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

dnolen00:12:22

I don't see a compelling reason to start now

dnolen00:12:47

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

richiardiandrea00:12:47

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

dnolen00:12:57

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

richiardiandrea00:12:11

well, it would have saved my time for sure 😉

dnolen00:12:34

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

dnolen00:12:37

I haven't seen much of it

richiardiandrea00:12:33

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...

dnolen00:12:52

but then other people want to mark such things

dnolen00:12:00

so I don't see a need to encourage that

dnolen00:12:14

this one just seems too edge-casey sorry

richiardiandrea00:12:51

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

richiardiandrea00:12:08

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

dnolen00:12:17

which lib?

dnolen00:12:31

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

dnolen00:12:12

also the PR is a context-less info hole

dnolen00:12:14

just saying

dnolen00:12:39

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

richiardiandrea00:12:00

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 😄

richiardiandrea00:12:09

so there is this bindings

richiardiandrea00:12:24

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

richiardiandrea00:12:35

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

richiardiandrea00:12:00

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

richiardiandrea00:12:22

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

dnolen00:12:39

but I mean affecting a lib adds more context about impact

dnolen00:12:49

i.e. sometimes something we do will break Reagent

dnolen00:12:51

and that's a showstopper

dnolen00:12:56

but you have to say that

dnolen00:12:24

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

dnolen00:12:27

it's not going to happen

dnolen00:12:33

please put all info in the PR

dnolen00:12:39

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

richiardiandrea00:12:44

added more info in the PR...

dnolen00:12:46

than just seeing it as abstract bug

dnolen00:12:03

and I know you mentioned it before here

richiardiandrea00:12:04

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

dnolen00:12:07

but Slack is transient

richiardiandrea00:12:18

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

dnolen00:12:18

and my brain isn't that big 🙂

dnolen00:12:33

thanks!

👍 4
richiardiandrea00:12:52

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 😄

dnolen14:12:13

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

mfikes14:12:02

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

dnolen14:12:05

cool, thanks!

borkdude16:12:46

@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.

dnolen16:12:05

@borkdude I unassigned 2955, doesn't apply anymore

borkdude16:12:22

maybe assign @mfikes since he wrote the patch?

borkdude16:12:34

anyway, now he probably saw it 🙂

lilactown18:12:15

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

dnolen18:12:41

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

dnolen18:12:29

I don't think we can make that work

lilactown18:12:29

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

dnolen18:12:50

in Clojure is datafy completely outside of core?

dnolen18:12:52

yes or no?

dnolen18:12:01

if no, then you cannot make it work

dnolen18:12:19

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

Alex Miller (Clojure team)18:12:35

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

lilactown18:12:38

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

dnolen18:12:53

then yes, we should rearrange

dnolen18:12:09

cljs.datafy + cljs.core.protocols

dnolen18:12:18

which would aid portability

dnolen18:12:33

due to auto-aliasing of clojure nses

lilactown18:12:13

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

dnolen18:12:20

oh rewinding

dnolen18:12:23

what's the problem here?

dnolen18:12:34

we have clojure.datafy already in ClojureScript

dnolen18:12:56

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

dnolen18:12:05

I suppose that could even be clojure.core.protocols

lilactown18:12:54

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

dnolen18:12:06

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

lilactown18:12:47

we probably want to do the same for Navigable

mfikes19:12:32

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

mfikes20:12:45

@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.

lilactown20:12:35

do we currently extend any protocols to JS errors?

lilactown20:12:43

looking for examples

mfikes20:12:58

Not that I’m aware of

lilactown20:12:03

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

dnolen20:12:06

Java class stuff is meaningless

dnolen20:12:58

so let's put that to one side

dnolen20:12:12

what is using the class intended to do?

lilactown20:12:53

reading into it, I think the intention for the ::class metadata is to at-a-glance be able to switch depending on what the type of the original value was that was datafied

lilactown20:12:38

e.g. if you have multiple values that get datafied to maps, it would be useful to know what the original value was to do things with it later

lilactown20:12:32

like hydrate it to the original value, perhaps

dnolen21:12:59

I don't think so

dnolen21:12:05

I looked at the git history

dnolen21:12:21

let's just ignore this for now

dnolen21:12:26

actually, put the code there but comment it out for now

dnolen21:12:31

and we can circle back later

lilactown21:12:50

well I’m still trying to think of how to do it in a way that works for all environments and with advanced compilation

lilactown21:12:57

ideally we could use x.constructor.name but I think that doesn’t work in < IE 9

lilactown21:12:57

i’ll just leave it at that but commented out for now

dnolen21:12:08

yeah I wouldn't spend any time on this right now

dnolen20:12:15

that's all that matters

mfikes20:12:29

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

mfikes20:12:12

(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.)

mfikes20:12:54

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

dnolen21:12:18

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

dnolen21:12:26

all other phases are in the analyzer, compiler or repl

mfikes21:12:53

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.

mfikes21:12:07

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

dnolen21:12:52

actually that true

mfikes21:12:54

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

dnolen21:12:07

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

dnolen21:12:26

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

mfikes21:12:31

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

dnolen21:12:49

oh I see, already done

dnolen21:12:59

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

mfikes21:12:15

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.

dnolen21:12:19

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

dnolen21:12:41

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

dnolen21:12:55

stacktrace.cljc

dnolen21:12:20

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

dnolen21:12:54

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

mfikes21:12:57

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

dnolen21:12:02

merge this one and I can do that wiring

mfikes21:12:22

Ok. Will do.

mfikes21:12:37

Just running tests before merging...

dnolen21:12:02

@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?

dnolen21:12:11

(also nav)

lilactown21:12:38

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

dnolen21:12:42

ok cool 🙂

mfikes21:12:04

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

mfikes21:12:05

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