LightTable / Clojure

Light Table Clojure language plugin
MIT License
99 stars 47 forks source link

Eval whole clojurescript file without "already declared" exception #28

Closed cldwalker closed 10 years ago

cldwalker commented 10 years ago

This is a work-in-progress pull at re-evaling a clojurescript file without getting a "namespace NS already declared" exception (screenshot). The current file eval confuses users and discourages us from having reload workflows like figwheel does. This pull currently solves this by providing a command to eval a file's contents without the ns form. However, I plan to extend this so that by default :eval uses the new behavior for cljs files. I realize this is not desirable for clojurescript files connected the LT-UI client since the LT eval is able to handle ns evals fine.

Things left to do on this pull:

Feedback from cljs users welcome. @ibdknox If you have any concerns, I'm interested to hear them.

ibdknox commented 10 years ago

I don't think this is the right way to fix this one. As I recall, the error is caused just by a flag being set when the cljs compiler has compiled in a certain way. At the beginning of the compilation you'll see something like this:

var COMPILED = !0, goog = goog || {};
goog.global = this;
goog.DEBUG = !0;
goog.LOCALE = "en";
goog.TRUSTED_SITE = !0;
goog.provide = function(a) {
  if (!COMPILED) {
    if (goog.isProvided_(a)) {
      throw Error('Namespace "' + a + '" already declared.');
    }
    delete goog.implicitNamespaces_[a];
    for (var b = a;(b = b.substring(0, b.lastIndexOf("."))) && !goog.getObjectByName(b);) {
      goog.implicitNamespaces_[b] = !0;
    }
  }
  goog.exportPath_(a);
};
cldwalker commented 10 years ago

@ibdknox Thanks for the pointer. Does the new commit seem reasonable? I can split out ::no-eval into clj and cljs if that makes this more maintainable. Alternatively, I could try to only set this after a user visits a browser page though I don't if we have that event for external browsers. As for going with a configuration solution, I spent 20 minutes trying to configure COMPILED with :closure-defines and was not able to get it to work. From reading the goog compiler source, I'm not even sure if clojurescript supports setting COMPILED. Regardless, I'd prefer we just handle this for every user rather then tell them to set a flag which isn't documented in clojurescript.

ibdknox commented 10 years ago

Yeah this is better. I wonder if this is an argument for starting to split these into two different behaviors though now that there's starting to be more language specific stuff going on. That can be done later though I guess.

cldwalker commented 10 years ago

Agreed on splitting them up so I did. While splitting out cljs I noticed the eval-location behavior is just commented out code. Any objection if I rip it out and its references in the nrepl jar? Going to include this pull in the release I'm cutting in a couple of hours unless there's a concern.