ClojureScript

Errors that occour in a ClojureScript pREPL connection are :ret as data under :val, not as a string

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: 1.10.238
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Environment:
    Arch Linux x86_64
  • Patch:
    Code
  • Approval:
    Accepted

Description

I'm working on some pREPL based tooling and noticed that if a JVM Clojure pREPL throws an error it is sent to stderr as well as a `:ret` under the `:val` keyword as a string.

When ClojureScript throws an error (in this case within nodejs) there is no stderr output and the `:val` contains actual EDN data, not a string. This inconsistency breaks the assumption that `:val` will always be a string of EDN data.

I've never raised an issue with Clojure before or submitted a patch, I am happy to do so if this is deemed a valid issue and worth fixing.

Activity

Hide
David Nolen added a comment -

I'm pretty sure the Clojure behavior is intentional - we should align.

Show
David Nolen added a comment - I'm pretty sure the Clojure behavior is intentional - we should align.
Hide
Oliver Caldwell added a comment -

I'm happy to do this, should I submit the patch through here or can I fork the ClojureScript repository? I've never contributed to any core repository before. I may have to sign a CLA?

Show
Oliver Caldwell added a comment - I'm happy to do this, should I submit the patch through here or can I fork the ClojureScript repository? I've never contributed to any core repository before. I may have to sign a CLA?
Hide
Mike Fikes added a comment -

Oliver, see the links under the Development section at https://clojurescript.org/community/resources. Yes a CA is required, which is covered in those links.

Show
Mike Fikes added a comment - Oliver, see the links under the Development section at https://clojurescript.org/community/resources. Yes a CA is required, which is covered in those links.
Hide
Oliver Caldwell added a comment -

So I think I understand the issue now, the errors slip through because `eval-cljs` always return a string but still throws exceptions, which we catch and return through :val: https://github.com/clojure/clojurescript/blob/6ccb629e365f46a9516e4defeced652cce9d4d35/src/main/clojure/cljs/core/server.clj#L108

I think there's two ways around this:

1. We catch as early as possible and ensure these exceptions are reported as strings. (via pr-str)
2. We catch as late as possible and upgrade the valf function to pr-str things that aren't already strings: https://github.com/clojure/clojurescript/blob/6ccb629e365f46a9516e4defeced652cce9d4d35/src/main/clojure/cljs/core/server.clj#L146

I personally think we should just pr-str (as well as Throwable->map) at the time they're caught and placed under a :val key. This is what Clojure does as well as setting :exception true on the response. Thoughts?

Show
Oliver Caldwell added a comment - So I think I understand the issue now, the errors slip through because `eval-cljs` always return a string but still throws exceptions, which we catch and return through :val: https://github.com/clojure/clojurescript/blob/6ccb629e365f46a9516e4defeced652cce9d4d35/src/main/clojure/cljs/core/server.clj#L108 I think there's two ways around this: 1. We catch as early as possible and ensure these exceptions are reported as strings. (via pr-str) 2. We catch as late as possible and upgrade the valf function to pr-str things that aren't already strings: https://github.com/clojure/clojurescript/blob/6ccb629e365f46a9516e4defeced652cce9d4d35/src/main/clojure/cljs/core/server.clj#L146 I personally think we should just pr-str (as well as Throwable->map) at the time they're caught and placed under a :val key. This is what Clojure does as well as setting :exception true on the response. Thoughts?
Hide
Oliver Caldwell added a comment -

https://github.com/clojure/clojure/blob/28b87d53909774af28f9f9ba6dfa2d4b94194a57/src/clj/clojure/core/server.clj#L247-L258

The above is the code I think we should align with but with an additional valf for exceptions that defaults to converting it to data and then into a string.

Show
Oliver Caldwell added a comment - https://github.com/clojure/clojure/blob/28b87d53909774af28f9f9ba6dfa2d4b94194a57/src/clj/clojure/core/server.clj#L247-L258 The above is the code I think we should align with but with an additional valf for exceptions that defaults to converting it to data and then into a string.
Hide
Oliver Caldwell added a comment -

This updates the default valf to pr-str if it encounters a non-string. At the moment, this will only ever be a map returned by Throwable->map.

Show
Oliver Caldwell added a comment - This updates the default valf to pr-str if it encounters a non-string. At the moment, this will only ever be a map returned by Throwable->map.
Hide
Oliver Caldwell added a comment -

I've attached one way to fix this, happy to approach it another way if you don't like it!

Show
Oliver Caldwell added a comment - I've attached one way to fix this, happy to approach it another way if you don't like it!
Hide
Mike Fikes added a comment -

CLJS-2994.patch passes CI

Show
Mike Fikes added a comment - CLJS-2994.patch passes CI
Hide
David Nolen added a comment -

Oliver, I believe this patch allows more than the Clojure pREPL, yes? i.e. the valf customization bit?

Show
David Nolen added a comment - Oliver, I believe this patch allows more than the Clojure pREPL, yes? i.e. the valf customization bit?
Hide
Oliver Caldwell added a comment -

Not as far as I know, unless I'm misunderstanding your question. Clojure also allows you to provide valf to io-prepl which defaults to pr-str.

I've tried to align this as closely as I can, the main difference here is that ClojureScript eval already returns results as strings, so identity is okay in most cases. When our prepl returns exceptions however we now pr-str those.

Show
Oliver Caldwell added a comment - Not as far as I know, unless I'm misunderstanding your question. Clojure also allows you to provide valf to io-prepl which defaults to pr-str. I've tried to align this as closely as I can, the main difference here is that ClojureScript eval already returns results as strings, so identity is okay in most cases. When our prepl returns exceptions however we now pr-str those.
Hide
David Nolen added a comment -

Thanks, I double checked and can confirm. Looks good to me.

Show
David Nolen added a comment - Thanks, I double checked and can confirm. Looks good to me.

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: