New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ClojureScript: C-x C-e does not print results nor exceptions #830

Closed
danskarda opened this Issue Sep 26, 2014 · 25 comments

Comments

Projects
None yet
6 participants
@danskarda
Contributor

danskarda commented Sep 26, 2014

Cider stopped to print results of eval last expression from ClojureScript buffer.

Emacs just prints "nil" in the minibuffer. The result of evaluation is not printed in repl buffer. The only workaround is to use (prn ...). Same applies to JS exceptions (they are lost). Clojure evaluation works fine.

Setup: latest cider / cider-nrepl. Latest release of ClojureScript and piggieback. I tried different backends (austin with chrome, cljs-noderepl etc)

There are many dependencies which can go wrong. At this point I am not able to point my finger on exact cause. I try to investigate the issue over weekend.

@danskarda

This comment has been minimized.

Show comment
Hide comment
@danskarda

danskarda Sep 26, 2014

Contributor

I found the root cause. I have a fix for piggieback, but I am not 100% happy with the solution. It smells more like a workaround than a solution and can result in some performance problems. Final solution should be coordinated between cider, nrepl and piggieback.

Some time ago cider stopped to use nrepl eval. It uses load-file instead. There were probably some good reasons for that in Clojure, but in ClojureScript load-file does not return value of last expression (as Clojure does).

Piggieback's load file uses cljs.repl/load-stream. The function uses doseq so the last value is lost. This can be fixed using map+last. However the fix will not solve the problem completely. Plain evaluate-form uses .toString, so objects are returned as [Object object].

One could patch load-stream to use print last expression using pr-str. Unfortunatelly there is a catch. ClojureScript def does return a value. For huge objects pr-str can be very expensive. If I patch piggieback on cljs.repl to pr-str last expression, C-x C-k (load current buffer) can block repl for very very very long time if the last expression was (def x (make-something-big)).

I think the core problem is that eval API in nrepl was not suitable for all use-cases in cider. So cider started to use load-file for both eval and load-file. I guess it was a workaround. It was faster to implement than to change nrepl eval API (and possibly breake other apps).

Solutions:

  1. reimplement load-stream in piggieback to print last expressions (and break loading of namespaces, where the last value is a large data structure)
  2. use eval to evaluate one expression and load-file to load files and namespaces. This would probably require changes in tools.nrepl, so it fits all cider use cases. In the long run it would probably help other apps as well.

What do you think? (sorry for long post)

/cc @cemerick @bbatsov

Contributor

danskarda commented Sep 26, 2014

I found the root cause. I have a fix for piggieback, but I am not 100% happy with the solution. It smells more like a workaround than a solution and can result in some performance problems. Final solution should be coordinated between cider, nrepl and piggieback.

Some time ago cider stopped to use nrepl eval. It uses load-file instead. There were probably some good reasons for that in Clojure, but in ClojureScript load-file does not return value of last expression (as Clojure does).

Piggieback's load file uses cljs.repl/load-stream. The function uses doseq so the last value is lost. This can be fixed using map+last. However the fix will not solve the problem completely. Plain evaluate-form uses .toString, so objects are returned as [Object object].

One could patch load-stream to use print last expression using pr-str. Unfortunatelly there is a catch. ClojureScript def does return a value. For huge objects pr-str can be very expensive. If I patch piggieback on cljs.repl to pr-str last expression, C-x C-k (load current buffer) can block repl for very very very long time if the last expression was (def x (make-something-big)).

I think the core problem is that eval API in nrepl was not suitable for all use-cases in cider. So cider started to use load-file for both eval and load-file. I guess it was a workaround. It was faster to implement than to change nrepl eval API (and possibly breake other apps).

Solutions:

  1. reimplement load-stream in piggieback to print last expressions (and break loading of namespaces, where the last value is a large data structure)
  2. use eval to evaluate one expression and load-file to load files and namespaces. This would probably require changes in tools.nrepl, so it fits all cider use cases. In the long run it would probably help other apps as well.

What do you think? (sorry for long post)

/cc @cemerick @bbatsov

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Sep 30, 2014

Member

Some time ago cider stopped to use nrepl eval. It uses load-file instead. There were probably some good reasons for that in Clojure, but in ClojureScript load-file does not return value of last expression (as Clojure does).

This sounds like a bug to me.

I think the core problem is that eval API in nrepl was not suitable for all use-cases in cider. So cider started to use load-file for both eval and load-file. I guess it was a workaround. It was faster to implement than to change nrepl eval API (and possibly breake other apps).

See NREPL-59. Using eval is not really an option as outlined there and load-file should in theory work fine for both Clojure and ClojureScript code.

use eval to evaluate one expression and load-file to load files and namespaces. This would probably require changes in tools.nrepl, so it fits all cider use cases. In the long run it would probably help other apps as well.

Doubt this is going to happen, so I think option 1 is the only realistic solution.

Member

bbatsov commented Sep 30, 2014

Some time ago cider stopped to use nrepl eval. It uses load-file instead. There were probably some good reasons for that in Clojure, but in ClojureScript load-file does not return value of last expression (as Clojure does).

This sounds like a bug to me.

I think the core problem is that eval API in nrepl was not suitable for all use-cases in cider. So cider started to use load-file for both eval and load-file. I guess it was a workaround. It was faster to implement than to change nrepl eval API (and possibly breake other apps).

See NREPL-59. Using eval is not really an option as outlined there and load-file should in theory work fine for both Clojure and ClojureScript code.

use eval to evaluate one expression and load-file to load files and namespaces. This would probably require changes in tools.nrepl, so it fits all cider use cases. In the long run it would probably help other apps as well.

Doubt this is going to happen, so I think option 1 is the only realistic solution.

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Sep 30, 2014

Member

One could patch load-stream to use print last expression using pr-str. Unfortunatelly there is a catch. ClojureScript def does return a value. For huge objects pr-str can be very expensive. If I patch piggieback on cljs.repl to pr-str last expression, C-x C-k (load current buffer) can block repl for very very very long time if the last expression was (def x (make-something-big)).

Yeah, I get this, but it seems to me that having consistent behaviour in both Clojure and ClojureScript is important. I doubt that this would be significant problem in practice.

Member

bbatsov commented Sep 30, 2014

One could patch load-stream to use print last expression using pr-str. Unfortunatelly there is a catch. ClojureScript def does return a value. For huge objects pr-str can be very expensive. If I patch piggieback on cljs.repl to pr-str last expression, C-x C-k (load current buffer) can block repl for very very very long time if the last expression was (def x (make-something-big)).

Yeah, I get this, but it seems to me that having consistent behaviour in both Clojure and ClojureScript is important. I doubt that this would be significant problem in practice.

@bbatsov bbatsov modified the milestone: v0.8 Oct 28, 2014

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Oct 31, 2014

Member

@cemerick Any thoughts on this?

Member

bbatsov commented Oct 31, 2014

@cemerick Any thoughts on this?

@danskarda

This comment has been minimized.

Show comment
Hide comment
@danskarda

danskarda Nov 1, 2014

Contributor

One last thing I forgot to wrote. In ClojureScript, load-file is slower than eval (austin + piggieback + chrome). You can notice the difference between working in repl (eval, instantaneous) and buffer (load-file, 1-2secs). Looks similar to issue #861 (though the reason will be CLJS specific).

It seems that load-file brings in as many problems as it is trying to address :(

Contributor

danskarda commented Nov 1, 2014

One last thing I forgot to wrote. In ClojureScript, load-file is slower than eval (austin + piggieback + chrome). You can notice the difference between working in repl (eval, instantaneous) and buffer (load-file, 1-2secs). Looks similar to issue #861 (though the reason will be CLJS specific).

It seems that load-file brings in as many problems as it is trying to address :(

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Nov 1, 2014

Member

@danskarda In #861 using in-ns instead of ns in the generated file to eval solves the speed problem. Does this improve performance for ClojureScript as well?

Member

bbatsov commented Nov 1, 2014

@danskarda In #861 using in-ns instead of ns in the generated file to eval solves the speed problem. Does this improve performance for ClojureScript as well?

@danskarda

This comment has been minimized.

Show comment
Hide comment
@danskarda

danskarda Nov 1, 2014

Contributor

@bbatsov There is no in-ns in ClojureScript. It is implemented as a special form. Special forms are interpreted in cljs.repl/repl and available only to repl (and indirectly to eval op). It is not available to cljs.repl/evaluate-form nor load-file op.

I think I could change it piggieback, but I feel like navigating inside a labyrinth. You patch one thing and many other things backfire on you. Not good. But I will try.

Contributor

danskarda commented Nov 1, 2014

@bbatsov There is no in-ns in ClojureScript. It is implemented as a special form. Special forms are interpreted in cljs.repl/repl and available only to repl (and indirectly to eval op). It is not available to cljs.repl/evaluate-form nor load-file op.

I think I could change it piggieback, but I feel like navigating inside a labyrinth. You patch one thing and many other things backfire on you. Not good. But I will try.

@danskarda

This comment has been minimized.

Show comment
Hide comment
@danskarda

danskarda Nov 22, 2014

Contributor

@bbatsov Support of in-ns during loading was not hard at the end. I tested it with recent cider and after change in cider--dummy-file-contents it is indeed faster than original ns.

Unfortunately this fix will not be available to 0.8 users until @cemerick makes new piggieback release and you change ns to in-ns.

ps: I just saw your video from Clojure/conj on youtube. Great talk! thumbs up Will the unsession also be available?

Contributor

danskarda commented Nov 22, 2014

@bbatsov Support of in-ns during loading was not hard at the end. I tested it with recent cider and after change in cider--dummy-file-contents it is indeed faster than original ns.

Unfortunately this fix will not be available to 0.8 users until @cemerick makes new piggieback release and you change ns to in-ns.

ps: I just saw your video from Clojure/conj on youtube. Great talk! thumbs up Will the unsession also be available?

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Nov 22, 2014

Member

Unfortunately this fix will not be available to 0.8 users until @cemerick makes new piggieback release and you change ns to in-ns.

I'll do the CIDER change immediately after the new piggieback is out (and I'll cut a bugfix release).

Will the unsession also be available?

They didn't record the unsessions.

Member

bbatsov commented Nov 22, 2014

Unfortunately this fix will not be available to 0.8 users until @cemerick makes new piggieback release and you change ns to in-ns.

I'll do the CIDER change immediately after the new piggieback is out (and I'll cut a bugfix release).

Will the unsession also be available?

They didn't record the unsessions.

@swannodette

This comment has been minimized.

Show comment
Hide comment
@swannodette

swannodette Nov 23, 2014

Just chiming in to say it's not clear to me what is actionable from a ClojureScript compiler point of view. If there is can someone please summarize? Thanks.

Just chiming in to say it's not clear to me what is actionable from a ClojureScript compiler point of view. If there is can someone please summarize? Thanks.

@danskarda

This comment has been minimized.

Show comment
Hide comment
@danskarda

danskarda Nov 23, 2014

Contributor

@swannodette At this moment, there is nothing actionable from ClojureScript compiler POV..

Summary: CIDER uses load-file to evaluate expressions. It solves line/row number reporting (see NREPL-50) by producing file with ns. blank lines and the expression. This solution was fine for Clojure, because it returns a value of the last expression in a loaded file.

ClojureScript REPL does not return last value and for good reason. It would have to print and parse a value of the last expression. This might be very expensive for complex data structures.

So I made two changes to piggieback: load-file returns a value of last expression and interprets in-ns during file loading (speed workaround, because in-ns is way faster than ns).

I deliberately have not implemented the changes in ClojureScript itself. It would not be OK to wrap last expressions in all files with prstr just because some IDE might need it. Piggieback is better place for this.

Personally, I think using load-file middleware to evaluate expression (when you already have eval expression middleware) does not sound right and is inviting new unforeseen issues. My patch is mere workaround to fix both issues.

Contributor

danskarda commented Nov 23, 2014

@swannodette At this moment, there is nothing actionable from ClojureScript compiler POV..

Summary: CIDER uses load-file to evaluate expressions. It solves line/row number reporting (see NREPL-50) by producing file with ns. blank lines and the expression. This solution was fine for Clojure, because it returns a value of the last expression in a loaded file.

ClojureScript REPL does not return last value and for good reason. It would have to print and parse a value of the last expression. This might be very expensive for complex data structures.

So I made two changes to piggieback: load-file returns a value of last expression and interprets in-ns during file loading (speed workaround, because in-ns is way faster than ns).

I deliberately have not implemented the changes in ClojureScript itself. It would not be OK to wrap last expressions in all files with prstr just because some IDE might need it. Piggieback is better place for this.

Personally, I think using load-file middleware to evaluate expression (when you already have eval expression middleware) does not sound right and is inviting new unforeseen issues. My patch is mere workaround to fix both issues.

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Nov 24, 2014

Member

ClojureScript REPL does not return last value and for good reason. It would have to print and parse a value of the last expression. This might be very expensive for complex data structures.

Forgive my ignorance, but why is this acceptable for Clojure and not for ClojureScript? That doesn't make any sense to me and as a tool writer I'd certainly prefer if load-file actually returned some result.

I deliberately have not implemented the changes in ClojureScript itself. It would not be OK to wrap last expressions in all files with prstr just because some IDE might need it. Piggieback is better place for this.

For the in-ns changes - sure. But there's a significant benefit to using ns instead of in-ns in some contexts - notably you don't have to worry whether your ns in some file is actually evaled or not. So it seems to me that finding a way to speed up ns (if that's possible at all) would be a better solution in the long term.

Personally, I think using load-file middleware to evaluate expression (when you already have eval expression middleware) does not sound right and is inviting new unforeseen issues. My patch is mere workaround to fix both issues.

I agree it sounds a bit odd and we'll probably modify the eval middleware at some point, but it gets the job done. Having proper metadata is super important and obviously eval can't deliver that right now.

Member

bbatsov commented Nov 24, 2014

ClojureScript REPL does not return last value and for good reason. It would have to print and parse a value of the last expression. This might be very expensive for complex data structures.

Forgive my ignorance, but why is this acceptable for Clojure and not for ClojureScript? That doesn't make any sense to me and as a tool writer I'd certainly prefer if load-file actually returned some result.

I deliberately have not implemented the changes in ClojureScript itself. It would not be OK to wrap last expressions in all files with prstr just because some IDE might need it. Piggieback is better place for this.

For the in-ns changes - sure. But there's a significant benefit to using ns instead of in-ns in some contexts - notably you don't have to worry whether your ns in some file is actually evaled or not. So it seems to me that finding a way to speed up ns (if that's possible at all) would be a better solution in the long term.

Personally, I think using load-file middleware to evaluate expression (when you already have eval expression middleware) does not sound right and is inviting new unforeseen issues. My patch is mere workaround to fix both issues.

I agree it sounds a bit odd and we'll probably modify the eval middleware at some point, but it gets the job done. Having proper metadata is super important and obviously eval can't deliver that right now.

@swannodette

This comment has been minimized.

Show comment
Hide comment
@swannodette

swannodette Nov 24, 2014

There's no good way to make ClojureScript return the last value from a ClojureScript file (if somebody has a brilliant idea I'm all ears). It's highly unlikely that we'll do anything about this.

There's no good way to make ClojureScript return the last value from a ClojureScript file (if somebody has a brilliant idea I'm all ears). It's highly unlikely that we'll do anything about this.

@danskarda

This comment has been minimized.

Show comment
Hide comment
@danskarda

danskarda Nov 24, 2014

Contributor

@bbatsov The difference between CLJ and CLJS: nREPL is CLJ, so when load-file returns a value, it is passed around inside nREPL without printing and parsing (the fact that you need to present it to the user from cider is a different thing - for sure you need to print it, but only when you really need it).

CLJS is different. If you need a value from load-file, you have to wrap last expression with pr-str, eval it on client (chrome, node.js, phantomjs etc) and send the value back to nREPL server. So if last file expression returns large complex value, CLJS pays a price for printing and parsing, while CLJ only passes a value around.

That's why we had to implement it in piggieback. If we did it directly in CLJS, all load-file ops would pay the price for printing, regardless if the value is usable or not.

There is one last difference between CLJ and CLJS. piggieback tries to parse the return CLJS value to CLJ value and if it is not able to do so (eg CLJS has a record which is not defined in CLJ), it returns the value as a output string. That's the reason why in CLJS simple values are printed in Emacs Minibuffer and "complex" to CIDER output buffer.

Contributor

danskarda commented Nov 24, 2014

@bbatsov The difference between CLJ and CLJS: nREPL is CLJ, so when load-file returns a value, it is passed around inside nREPL without printing and parsing (the fact that you need to present it to the user from cider is a different thing - for sure you need to print it, but only when you really need it).

CLJS is different. If you need a value from load-file, you have to wrap last expression with pr-str, eval it on client (chrome, node.js, phantomjs etc) and send the value back to nREPL server. So if last file expression returns large complex value, CLJS pays a price for printing and parsing, while CLJ only passes a value around.

That's why we had to implement it in piggieback. If we did it directly in CLJS, all load-file ops would pay the price for printing, regardless if the value is usable or not.

There is one last difference between CLJ and CLJS. piggieback tries to parse the return CLJS value to CLJ value and if it is not able to do so (eg CLJS has a record which is not defined in CLJ), it returns the value as a output string. That's the reason why in CLJS simple values are printed in Emacs Minibuffer and "complex" to CIDER output buffer.

@cemerick

This comment has been minimized.

Show comment
Hide comment
@cemerick

cemerick Nov 25, 2014

Contributor

I'm not comfortable with the proposed changes to piggieback, for a couple of reasons.

First, pr-str-ing the last form being evaluated in the course of handling load-file there is no better than if ClojureScript itself did it. As @danskarda pointed out, all load-file ops will pay that price, just to support the particular set of objectives of cider-eval-last-sexp and similar (eval the form in question, applying file-accurate line numbers, and returning/printing the result). The nature of ClojureScript evaluation makes this infeasible in general, at least while we're abusing load-file.

Second, I don't want to further complicate piggieback to accommodate that abuse; the "in-sourcing" of load-stream in the proposed patch is a big warning flag for me, as that could easily produce compatibility issues in the future.

Until someone does the work to add file/line/column options to both nREPL's and piggieback's eval, the current hack using load-file will have to suffice (and as I mentioned here, that's probably going to be a delicate piece of work). The tradeoff then is whether tools want to prioritize file-accurate line numbers or receiving/displaying results when performing eval-expr-at-point sorts of operations in ClojureScript; if the latter is deemed more important, then perhaps this class of commands should use eval for ClojureScript, and load-file for Clojure.

Sorry for the perhaps disappointing response. I now regret suggesting the load-file hack in the first place; the energy spent on it to date probably would have been sufficient to solve the problem correctly.

Contributor

cemerick commented Nov 25, 2014

I'm not comfortable with the proposed changes to piggieback, for a couple of reasons.

First, pr-str-ing the last form being evaluated in the course of handling load-file there is no better than if ClojureScript itself did it. As @danskarda pointed out, all load-file ops will pay that price, just to support the particular set of objectives of cider-eval-last-sexp and similar (eval the form in question, applying file-accurate line numbers, and returning/printing the result). The nature of ClojureScript evaluation makes this infeasible in general, at least while we're abusing load-file.

Second, I don't want to further complicate piggieback to accommodate that abuse; the "in-sourcing" of load-stream in the proposed patch is a big warning flag for me, as that could easily produce compatibility issues in the future.

Until someone does the work to add file/line/column options to both nREPL's and piggieback's eval, the current hack using load-file will have to suffice (and as I mentioned here, that's probably going to be a delicate piece of work). The tradeoff then is whether tools want to prioritize file-accurate line numbers or receiving/displaying results when performing eval-expr-at-point sorts of operations in ClojureScript; if the latter is deemed more important, then perhaps this class of commands should use eval for ClojureScript, and load-file for Clojure.

Sorry for the perhaps disappointing response. I now regret suggesting the load-file hack in the first place; the energy spent on it to date probably would have been sufficient to solve the problem correctly.

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Nov 25, 2014

Member

Sorry for the perhaps disappointing response. I now regret suggesting the load-file hack in the first place; the energy spent on it to date probably would have been sufficient to solve the problem correctly.

Perhaps. :-) Anyways, someone else would have to deal with this as I'm way too busy with other stuff.
While we can add some client-side workaround (like some config whether to use eval or load-file) I'm opposed to this idea and would like to see an actual solution to this problem.

Member

bbatsov commented Nov 25, 2014

Sorry for the perhaps disappointing response. I now regret suggesting the load-file hack in the first place; the energy spent on it to date probably would have been sufficient to solve the problem correctly.

Perhaps. :-) Anyways, someone else would have to deal with this as I'm way too busy with other stuff.
While we can add some client-side workaround (like some config whether to use eval or load-file) I'm opposed to this idea and would like to see an actual solution to this problem.

@danskarda

This comment has been minimized.

Show comment
Hide comment
@danskarda

danskarda Nov 26, 2014

Contributor

This is a little bit unfortunate. CIDER is after release declared most stable, but for CLJS it does not show results of evaluation not even exceptions and errors. You could imagine how satisfying is doing development in such environment.

Could we agree on a plan and timeline for this bug? For example accept the patch as temporary workaround and than switch to perfect solution once it is ready? This bug is 2 months old. Given that we are all busy, I am worried that it will be not be solved in next few months.

Possible solutions:

  1. replace load-file with eval, but continue to use blank lines and white space. Would it still give correct line position?
  2. move load-file workaround to cljs-tooling (@bbatsov - can we somehow detect clojure and clojurescript evaluation and dispatch different calls based on the backend)?
  3. ???

Do you have other suggestions?

Contributor

danskarda commented Nov 26, 2014

This is a little bit unfortunate. CIDER is after release declared most stable, but for CLJS it does not show results of evaluation not even exceptions and errors. You could imagine how satisfying is doing development in such environment.

Could we agree on a plan and timeline for this bug? For example accept the patch as temporary workaround and than switch to perfect solution once it is ready? This bug is 2 months old. Given that we are all busy, I am worried that it will be not be solved in next few months.

Possible solutions:

  1. replace load-file with eval, but continue to use blank lines and white space. Would it still give correct line position?
  2. move load-file workaround to cljs-tooling (@bbatsov - can we somehow detect clojure and clojurescript evaluation and dispatch different calls based on the backend)?
  3. ???

Do you have other suggestions?

@cemerick

This comment has been minimized.

Show comment
Hide comment
@cemerick

cemerick Nov 26, 2014

Contributor

eval cannot be used for this functionality until the real solution to NREPL-59 is implemented: evaluations have no source file associated with them. The line number would be accurate, but useless.

Stepping back a bit, I wonder: do the real use cases for "eval expr at point" require all of the constraints I described earlier (eval the form in question, applying file-accurate line numbers, and returning/printing the result)? Thinking of my own workflow, I have two use cases:

  1. I have modified a top-level definition, and I want to load it, but not the whole file. In this case, I want accurate source (file/line/column) information, but the result of the evaluation is of no interest.
  2. I have modified something other than a definition, and want to evaluate it to see its value, but because it's not a definition, accurate source information isn't germane.

Are there really cases beyond these that demand both evaluation results and accurate source information?

Contributor

cemerick commented Nov 26, 2014

eval cannot be used for this functionality until the real solution to NREPL-59 is implemented: evaluations have no source file associated with them. The line number would be accurate, but useless.

Stepping back a bit, I wonder: do the real use cases for "eval expr at point" require all of the constraints I described earlier (eval the form in question, applying file-accurate line numbers, and returning/printing the result)? Thinking of my own workflow, I have two use cases:

  1. I have modified a top-level definition, and I want to load it, but not the whole file. In this case, I want accurate source (file/line/column) information, but the result of the evaluation is of no interest.
  2. I have modified something other than a definition, and want to evaluate it to see its value, but because it's not a definition, accurate source information isn't germane.

Are there really cases beyond these that demand both evaluation results and accurate source information?

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Nov 26, 2014

Member

Could we agree on a plan and timeline for this bug? For example accept the patch as temporary workaround and than switch to perfect solution once it is ready?

The problem with "temp" solutions is that they tend to stick around when nobody is feeling the urgency to fix them.

move load-file workaround to cljs-tooling (@bbatsov - can we somehow detect clojure and clojurescript evaluation and dispatch different calls based on the backend)?

We can do something like this in cider-nrepl, but this would tie even the most basic functionality (eval) to the presence of the middleware.

Guess @cemerick's idea might be an acceptable workaround, though - we can use eval for most evals and load-file just for top-level evals. This would have some implications (most people will likely be surprised that they produce different metadata and the value of eval-region would diminish), but this seems like the simplest short-term solution.

Member

bbatsov commented Nov 26, 2014

Could we agree on a plan and timeline for this bug? For example accept the patch as temporary workaround and than switch to perfect solution once it is ready?

The problem with "temp" solutions is that they tend to stick around when nobody is feeling the urgency to fix them.

move load-file workaround to cljs-tooling (@bbatsov - can we somehow detect clojure and clojurescript evaluation and dispatch different calls based on the backend)?

We can do something like this in cider-nrepl, but this would tie even the most basic functionality (eval) to the presence of the middleware.

Guess @cemerick's idea might be an acceptable workaround, though - we can use eval for most evals and load-file just for top-level evals. This would have some implications (most people will likely be surprised that they produce different metadata and the value of eval-region would diminish), but this seems like the simplest short-term solution.

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Dec 5, 2014

Member

I've updated the code - now only cider-eval-defun uses load-file.

Member

bbatsov commented Dec 5, 2014

I've updated the code - now only cider-eval-defun uses load-file.

@danskarda

This comment has been minimized.

Show comment
Hide comment
@danskarda

danskarda Dec 5, 2014

Contributor

@bbatsov Thank you!

Contributor

danskarda commented Dec 5, 2014

@bbatsov Thank you!

@scttnlsn

This comment has been minimized.

Show comment
Hide comment
@scttnlsn

scttnlsn Dec 5, 2014

@bbatsov It's working great. Thanks!

scttnlsn commented Dec 5, 2014

@bbatsov It's working great. Thanks!

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Dec 5, 2014

Member

@danskarda One problem remains, though - because of the lack of in-ns during load-file doing cider-eval-defun will remain slow. I see no reason why something like this should not be supported in piggieback.

Anyways, let's hope that eventually nREPL's eval and piggieback will be updated to support source tracking.

Member

bbatsov commented Dec 5, 2014

@danskarda One problem remains, though - because of the lack of in-ns during load-file doing cider-eval-defun will remain slow. I see no reason why something like this should not be supported in piggieback.

Anyways, let's hope that eventually nREPL's eval and piggieback will be updated to support source tracking.

@johnbendi

This comment has been minimized.

Show comment
Hide comment
@johnbendi

johnbendi Dec 6, 2014

Can't we just open a test branch on piggieback and test the recommendation off that?

Can't we just open a test branch on piggieback and test the recommendation off that?

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Dec 6, 2014

Member

Can't we just open a test branch on piggieback and test the recommendation off that?

We know that in-ns works as added by @danskarda. It's only a question of whether @cemerick will decide to merge this or not. See nrepl/piggieback#32.

Member

bbatsov commented Dec 6, 2014

Can't we just open a test branch on piggieback and test the recommendation off that?

We know that in-ns works as added by @danskarda. It's only a question of whether @cemerick will decide to merge this or not. See nrepl/piggieback#32.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment