Ahh it seems one reason this may have been missed is that it looks like some of this was recently changed in #12

@RickMoynihan The changes in #12 were simply cosmetic - a hardcoded function for printing the result was made configurable. It's still probably most prudent to simply insert this middleware before pr-values.

t seems this might be quite trivial by simply adding a new :actual-value v kvp here in addition to the existing :value which I'd propose is left as is for backwards compatibility.

We can do something like this, but there's a chance it can break some clients as a complex EDN response might be hard for them to swallow. I'll have to take a closer look at this.

Specifically what clients are you thinking @bbatsov?

The idea is that the :actual-value key would only stay in the nrepl server process middleware stack and would never be sent across the wire.

Are there any middlewares that we know of that look at all the keys from the response map?

I also suggested to Rich that it might be worth providing a new op:

rickmoynihan [4:59 PM]
I was meaning more outside of the REBL context. I understand we can’t for example serialise the fn > protocol extensions etc that REBL might want.
but yes what you say makes sense… It seems like we might be wanting a new :op "datafying-eval"

richhickey [4:59 PM]
yes, I think people might want that, just must be opt-in due to payload increase
remoting REBL will involve datafying fns such that they get weak-reference cached and a
reference-> key (data) is serialized, then a remote can send the reference-key back to invoke the fn,
thus datafy/nav can work remotely

With the idea being that a client could send a form to datafying-eval and have the server call datafy on the response before returning it.

If you think it's a good idea I can create a new ticket for it.

Specifically what clients are you thinking @bbatsov?

I recall at least in the early days of CIDER it's bencode parser would sometimes explode when faced with complex metadata EDN maps. And in general you can even represent everything can be in EDN in bencode, so this has to be kept in mind as well.

The idea is that the :actual-value key would only stay in the nrepl server process middleware stack and would never be sent across the wire.

That's kinda hard to achieve as eval would have to insert it and some other middleware has to disjoin in down the middleware stack, but if this middleware doesn't exist it will be sent to the clients. Of course, clients are not forced to process every key in the response map, but we're also not sure how they are implemented.

Are there any middlewares that we know of that look at all the keys from the response map?

There are many middlewares that simply inspect and modify a response map, but the real problems is what you end up sending to the clients in the end of the day. I'm guessing another option might be to store the EDN results server-side and have some op to retrieve them. I have to think more about this.

Rich's idea seems reasonable as well.

All of the above would be greatly simplified for clients of the upcoming EDN transport, but of course you'll need clients who can work with EDN. :-)

I'd also like to hear what @cgrand, @SevereOverfl0w and @alexander-yakushev think about this.

Without working with nREPL closely before, I'd say I never understood why it is so trigger-happy about turning the result values into a string early. So I think the approach Rick suggests is sensible. I would perhaps think of another name for it, :actual-value looks a bit non-informative, maybe something like :value-object might do better.

and some other middleware has to disjoin in down the middleware stack, but if this middleware doesn't exist it will be sent to the clients.

Couldn't there be some non-optional "middleware" that takes care of it and cannot be disabled?

trigger-happy about turning the result values into a string early.

Likely this wasn't always the case, as there are some historical references to :value and :printed-value. I wasn't involved with the project when it was decided to merged into one printed :value, but I assume this was done to simplify the lives of clients, as nothing as simple as encoding/decoding a string. The big problem with returning any EDN is that the client should also be capable of processing this EDN. And, as the this was never reported as a problem until datafy was introduced I guess it was a reasonable decision at the time. :-)

So I think the approach Rick suggests is sensible. I would perhaps think of another name for it, :actual-value looks a bit non-informative, maybe something like :value-object might do better.

Yeah, I don't like :actual-value either. It's a pity the ship with :value and :printed-value has sailed. I think :value-object is a good option. Or perhaps :value-edn or something along those lines. If someone has great idea about the name - I'm all ears.

Couldn't there be some non-optional "middleware" that takes care of it and cannot be disabled?

Yeah, this can certain be done. It would just have to be a middleware that comes after eval, but every other middleware that makes use of the value should know to insert itself between eval and it, which is not much more different that inserting yourself between eval and pr-values. It will certainly be easier to just keep the value around, but for that we'll certainly have to experiment with a few of the major clients to make sure they'll be fine like this.

Evaluation binds *1. So I suspect that you could access the actual result that way already. Something to consider. Having access to this would be useful though, I've run into this once before and fixed a fun bug.

Something to think about is whether pr-values was an architectural mistake. Maybe the transport should be responsible for this behavior? That is something that always happens at the "end".

The big problem with returning any EDN is that the client should also be capable of processing this EDN. And, as the this was never reported as a problem until datafy was introduced I guess it was a reasonable decision at the time. :-)

The big problem was (and still is but to a lesser extent) that printed values are not always readable (let alone EDN-readable) and even when they are readable, you lose information (e.g. class vs symbol). I know: I had to work around that for Unrepl.

That's why REBL is local (same JVM) too: it's just easier.

I'v started working on simple extension to Unrepl EDN representation to fully support datafy/nav, so you won't have to use a swing UI to get datafy/nav.

@SevereOverfl0w:

Something to think about is whether pr-values was an architectural mistake. Maybe the transport should be responsible for this behavior? That is something that always happens at the "end".

I strongly disagree. It's orthogonal to transport responsibility.

Current transport definition is already complected: why does bencode bundle socket transport and bencode representation together for example?

Current transport definition is already complected: why does bencode bundle socket transport and bencode representation together for example?

I agree. Likely this was something "to be improved" that simply never happened. I know Chas envisioned non-socket transports, so it certainly makes sense in principle to have clear separations of transports and representation.

The big problem was (and still is but to a lesser extent) that printed values are not always readable (let alone EDN-readable) and even when they are readable, you lose information (e.g. class vs symbol). I know: I had to work around that for Unrepl.

Great point.

EDIT: this comment is misleading/plain wrong; please see below.

@RickMoynihan:requires and :expects support either op names as strings or specific middleware as var references, so you can use :requires #{#'nrepl.middleware.pr-values/pr-values} to ensure the ordering you need.

We use the same technique of injecting a wrapping transport in both the pprint (impl / descriptor) and inspect(impl / descriptor) middlewares in cider-nrepl.

However @SevereOverfl0w raises a very good point:

Evaluation binds *1. So I suspect that you could access the actual result that way already.

Your middleware does not actually interfere with pr-values (e.g. by eliding :value from the response like in cider-nrepl's pprint middleware). You could skip the effort of injecting the transport by instead using :requires #{"eval"} :expects #{} in your descriptor. Now your handler will be called after evaluation and so the result will be bound in the session map in the request:

(let [{:keys [session]} request]
  (get @session #'*1))

Since the session middleware ensures serialised execution this is safe to do even in the face of concurrent eval requests being made.

@cichli Seems we can close this one, right? I guess we’ve decided that we don’t really need to do anything about it.

I meant to follow this up, I'd tried to do what @cichli said but had some trouble getting it to work. I'm not sure what happened (I suspect I wired things up wrong) but IIRC it looked to me like the middleware I wasn't able to access the *1 binding.

I suppose I'll need to try set this all up again, and maybe push it somewhere for someone to look at if I still can't get it to work.

Looks like this PR should fix the issue RickMoynihan/nrebl.middleware#17.

@RickMoynihan indeed, my comment above is incorrect on two counts. Sorry about that.

• A middleware using :requires #{"eval"} would not ever be invoked. You need to use :expects and assoc the wrapping transport.

• Since 64dafe7 we only reset! the session atom in the :prompt REPL phase. You could however read *1 directly, since the session's dynamic bindings are still in place when the response is handled. Arguably an implementation detail (what if Transport/send was changed to be async?) but highly unlikely to change – we rely on the bindings being in place in nREPL itself.

I opened the above linked PR to take the approach of enforcing the correct middleware ordering and using :value. On reflection this is more consistent with what other existing middlewares do.