<< Back to previous view

[CLJS-2344] de-dupe externs Created: 31/Aug/17  Updated: 21/Sep/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: Next

Type: Enhancement Priority: Minor
Reporter: David Nolen Assignee: Sameer Rahmani
Resolution: Unresolved Votes: 0
Labels: closure

Attachments: Text File CLJS-2344.patch    
Patch: Code
Approval: Vetted

 Description   

We could possibly call distinct on externs to remove dupes in the common case where externs may appear multiple times accidentally on the classpath.






[CLJS-2338] Add support for renamePrefix and renamePrefixNamespace closure compiler options Created: 29/Aug/17  Updated: 15/Sep/17  Resolved: 15/Sep/17

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Pieter du Toit Assignee: Pieter du Toit
Resolution: Completed Votes: 1
Labels: closure

Attachments: Text File CLJS-2338.patch     Text File CLJS-2338.patch    
Approval: Accepted

 Description   

Adding support for these closure compiler options would allow for a mechanism to avoid name collisions



 Comments   
Comment by Pieter du Toit [ 29/Aug/17 6:01 AM ]

Patch attached

Comment by David Nolen [ 29/Aug/17 6:35 AM ]

Pieter, thanks for the patch. Have you submitted your contributor agreement?

Comment by David Nolen [ 29/Aug/17 6:37 AM ]

One question I have about the patch, is there a default prefix name if not supplied by the user?

Comment by Pieter du Toit [ 29/Aug/17 7:05 AM ]

Yes, I have submitted my contributor agreement. The patch does not specify defaults, if the user does not supply :rename-prefix or :rename-prefix-namespace, then the compiler options passed to the closure compiler would be unchanged.

Comment by David Nolen [ 29/Aug/17 8:05 AM ]

Pieter sorry for being unclear, what happens if :rename-prefix-namespace is specified but :rename-prefix is not specified?

Comment by Pieter du Toit [ 29/Aug/17 9:40 AM ]

AFAICT, the options can be specified independently, the only documentation I could find on it is in Closure CommandLineRunner and here.

For my own use case, I only need to set :rename-prefix, which I have tested both with and without :modules and works as expected.

I did some further testing now with only specifying :rename-prefix-namespace (which calls setRenamePrefixNamespace), it has no impact on a build without :modules, and results in an error "{prefix} is not defined" when using with :modules. Specifying both :rename-prefix and :rename-prefix-namespace for a build with :modules results in the same "{prefix} is not defined" error.

I am less confident in :rename-prefix-namespace so I could modify the patch to remove the option ? I also noticed that I would need to modify the patch to add any new options to known-opts

Comment by Thomas Heller [ 29/Aug/17 11:26 AM ]

FWIW I'm using setRenamePrefixNamespace for a feature in shadow-cljs [1]. The effect is that only one global variable will be created and everything else will be a property of that variable. I'm using $CLJS as the prefix namespace and where :advanced mode would otherwise create a xY variable (just an example, name doesn't matter), it would now create a $CLJS.xY property instead.

I have never used :rename-prefix but my understanding is that if would just turn xY into $CLJSxY so you'd still end up with a bunch of global variables.

In my case I use it combined with :modules and only setRenamePrefixNamespace needs to be set and is otherwise unrelated to :rename-prefix.

[1] https://github.com/thheller/shadow-cljs/blob/21e9a3a279ed7a34239cc3c7cc65715f22289163/src/main/shadow/cljs/closure.clj#L481

Comment by David Nolen [ 30/Aug/17 6:21 AM ]

Ok so they sound like independent options then. Pieter, I'm fine with the patch granted it's updated to augment known-opts.

Comment by Pieter du Toit [ 30/Aug/17 9:01 AM ]

David, thanks, I have attached the updated patch

Comment by Pieter du Toit [ 30/Aug/17 9:17 AM ]

Thomas, thanks, I get the same result (no prefix namespace) with a shadow-cljs release build as with cljs (when using the new :rename-prefix-namespace and no :modules)

shadow-cljs.edn
{:dependencies
 []

 :source-paths
 ["src"]

 :builds
 {:app
  {:target :browser
   :output-dir "shadow/js"
   :modules
   {:main {:entries [moduleb]}}}}}
moduleb.cljs
(ns moduleb)

(def env "dev")

(defn init []
  (js/console.log (str "Module B init for env: " env)))

(init)
Comment by Thomas Heller [ 31/Aug/17 3:45 AM ]

The option itself is not supported as of now as I'm using the default cljs.closure/set-options function in shadow-cljs. When it is added to CLJS it will become available in shadow-cljs.

I'm currently using the feature internally for the :npm-module target which compiles code so it can be directly consumed by JS tools (or node). I'm setting that manually though so the :rename-prefix-namespace option is not recognized.

Comment by David Nolen [ 15/Sep/17 3:46 PM ]

fixed https://github.com/clojure/clojurescript/commit/7a8803ef70cb84c686341353e7ab29928487e388





[CLJS-2313] :language-out is a build affecting option Created: 09/Aug/17  Updated: 10/Aug/17  Resolved: 10/Aug/17

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.854
Fix Version/s: 1.9.908

Type: Defect Priority: Major
Reporter: David Nolen Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: closure, compiler

Attachments: Text File CLJS-2313.patch    
Patch: Code
Approval: Accepted

 Description   

We need to add this to the list.



 Comments   
Comment by David Nolen [ 10/Aug/17 6:26 AM ]

fixed https://github.com/clojure/clojurescript/commit/5d7fd9c1fbc7afb0b52ff581d1ec2d93f114ff0c





[CLJS-2311] Cut a Closure Library release Created: 08/Aug/17  Updated: 10/Aug/17  Resolved: 10/Aug/17

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: 1.9.908

Type: Task Priority: Major
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: closure

Approval: Accepted

 Comments   
Comment by David Nolen [ 10/Aug/17 6:57 AM ]

A new Closure Library release has been cut.





[CLJS-2225] Need to add :checked-arrays to known compiler opts Created: 13/Jul/17  Updated: 13/Jul/17  Resolved: 13/Jul/17

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.854
Fix Version/s: 1.9.854

Type: Defect Priority: Blocker
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: closure

Attachments: Text File CLJS-2225.patch    
Patch: Code
Approval: Accepted

 Description   

So, for example the compiler will warn if you misspell:

WARNING: Unknown compiler option ':checked-array'. Did you mean ':checked-arrays'?


 Comments   
Comment by David Nolen [ 13/Jul/17 4:48 PM ]

fixed https://github.com/clojure/clojurescript/commit/2247b0c2be4f2900bebfb544fea18ea3ed58f540





[CLJS-2181] Can't compile string sources with modules Created: 06/Jul/17  Updated: 07/Jul/17  Resolved: 07/Jul/17

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: 1.9.854

Type: Defect Priority: Minor
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: closure, modules

Attachments: Text File CLJS-2181.patch    
Patch: Code and Test
Approval: Accepted

 Comments   
Comment by David Nolen [ 07/Jul/17 2:03 PM ]

fixed https://github.com/clojure/clojurescript/commit/970b76c8b38e11420eb90d4ba4f4f4a9a2fc2a5e





[CLJS-2175] ES6 Module processing broken with Closure v20170626 Created: 05/Jul/17  Updated: 05/Jul/17  Resolved: 05/Jul/17

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.854
Fix Version/s: 1.9.854

Type: Defect Priority: Major
Reporter: Juho Teperi Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: closure

Attachments: Text File CLJS-2175-2.patch     Text File CLJS-2175.patch    
Approval: Vetted

 Description   

I have test project that was working previously, but doesn't work with cljs master. If I add dependency to [com.google.javascript/closure-compiler-unshaded "v20170521"] it works.

This will be good chance to add tests for ES6 processing: https://github.com/clojure/clojurescript/blob/master/src/test/clojure/cljs/module_processing_tests.clj



 Comments   
Comment by Juho Teperi [ 05/Jul/17 5:05 PM ]

This patch adds test case for ES6 processing. Currently test is broken.

Comment by Juho Teperi [ 05/Jul/17 5:08 PM ]

Added the missing test input file.

Comment by David Nolen [ 05/Jul/17 5:26 PM ]

fixed https://github.com/clojure/clojurescript/commit/e2fcea32ee234c3aaaf3ed0715401a8ce274eaf9





[CLJS-2161] Bump Closure Compiler to June 2017 release Created: 03/Jul/17  Updated: 03/Jul/17  Resolved: 03/Jul/17

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.854
Fix Version/s: 1.9.854

Type: Enhancement Priority: Minor
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: closure

Attachments: Text File CLJS-2161.patch    
Patch: Code
Approval: Vetted

 Description   

This release brings more improvements to the Closure Compiler module processing



 Comments   
Comment by David Nolen [ 03/Jul/17 2:44 PM ]

fixed https://github.com/clojure/clojurescript/commit/1d2ec690ca15276fa6ee3493d13c8ec133de9dbb





[CLJS-2156] Add postamble, or some other generic way to append code to a file Created: 03/Jul/17  Updated: 18/Aug/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.671
Fix Version/s: Next

Type: Enhancement Priority: Minor
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: closure

Approval: Vetted

 Description   

Generally useful, but specifically for appending clj.loader/set-loaded! calls to user files.



 Comments   
Comment by Thomas Heller [ 03/Jul/17 7:08 AM ]

In shadow-cljs I have 4 attributes per :module for this.

  • :prepend treated as text, eg. licence headers. Can contain JS but won't go through optimizations.
  • :prepend-js treated as JS code that will also go through closure optimizations
  • :append-js
  • :append

I think it is valuable to have all 4 and making the distinction between "text" and "JS". shadow.loader is implemented entirely through these attributes.

Comment by David Nolen [ 03/Jul/17 7:21 AM ]

I'm having second thoughts about this one, since I don't think we need this for CLJS-2157, which is why I opened it originally.

Comment by Thomas Heller [ 03/Jul/17 10:09 AM ]

set-loaded! should only be called once per module, so it must be appended to the module itself not to individual files in the module.

By appending to individual files you will eventually call it more than once for modules that have multiple entries. AFAICT the call is not idempotent and may cause events to be emitted multiple times. At the very least it may call set-loaded! before the full module has actually been loaded. Since it immediately triggers the callbacks that may lead to bad results.

The config options are generally useful not just for the loader.

Comment by David Nolen [ 03/Jul/17 10:44 AM ]

Thomas as far as I can tell ModuleManager.setLoaded is idempotent. I haven't run into any issues locally with testing. Feel free provide a failing case if you can but I couldn't find one myself.





[CLJS-2017] Upgrade Closure Compiler to latest April 2017 release Created: 25/Apr/17  Updated: 28/Apr/17  Resolved: 28/Apr/17

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: closure

Attachments: Text File CLJS-2017.patch    
Patch: Code

 Description   

The Closure Compiler team has released a new version that includes more enhancements to JS module processing



 Comments   
Comment by David Nolen [ 28/Apr/17 3:11 PM ]

fixed https://github.com/clojure/clojurescript/commit/c488bfed8d050d9825a7407dfc4bb813e2cdda85





[CLJS-1770] goog-defines broken for integers Created: 01/Sep/16  Updated: 16/Sep/16  Resolved: 16/Sep/16

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Tom Connors Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: closure

Attachments: Text File CLJS-1770.patch    

 Description   

Using goog-define with integer values results in cljs.closure/make-options attempting to call .setDefineToIntegerLiteral on the compiler-options object. That method does not exist; the correct method is .setDefineToNumberLiteral. Note however that calls to .setDefineToNumberLiteral will fail when the value is a long; it might make sense to always use .setDefineToDoubleLiteral when the value is a number. Integers passed to .setDefineToDoubleLiteral remain integers in the output javascript, so it seems harmless enough to do it that way.



 Comments   
Comment by Tom Connors [ 02/Sep/16 8:55 AM ]

Patch for using setDefineToDoubleLiteral for all numbers.

Comment by David Nolen [ 16/Sep/16 2:01 PM ]

fixed https://github.com/clojure/clojurescript/commit/1d8964ea632fbb3c775cbacb1f11807fbdfe9e72





[CLJS-1762] Bump Closure Compiler Created: 22/Aug/16  Updated: 18/Oct/16  Resolved: 18/Oct/16

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: 1.9.293

Type: Task Priority: Minor
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: closure

Attachments: Text File CLJS-1762-3.patch     Text File CLJS-1762-4.patch     Text File CLJS-1762.patch    

 Description   

A new version of the Closure Compiler features an optimization that results in faster compilation, as well as some some changes to ES6-related features.



 Comments   
Comment by Juho Teperi [ 27/Aug/16 4:39 AM ]

Here is my version of the change.

Updating cljs.closure was quite straightforward, just removed all the checks and changed a few constructor calls.

I think I found all places to update the Closure version: pom.template.xml, project.clj and script/bootstrap. Bootstrap script also needed a change to account for changes closure-compiler filename.

  • ES6ModuleLoader has been replaced with ModuleLoader, it no longer
    needs to be passed to ProcessCommonJSModules etc. constructors
  • This version requires the version 20160822 and any checks needed to
    work with older versions have been removed
  • Closure-compiler jar name has been changed so lib/ and closure/
    directories should be removed before running bootstrap
Comment by Juho Teperi [ 27/Aug/16 4:55 AM ]

While tests pass with the patch, looks like module processing is broken. Tested with https://github.com/mneise/circle-color

Comment by Juho Teperi [ 15/Sep/16 5:19 AM ]

Current WIP branch at https://github.com/clojure/clojurescript/compare/master...Deraen:cljs-1762

I started writing some tests for module processing based on Mneise's circle-color example. I heavily simplified React UMD module and JSX processing for the tests, but not yet very happy with this method. Probably best to write the JS for tests from scratch.

Currently stuck on getting Closure compiler to return the processed code: https://github.com/clojure/clojurescript/compare/master...Deraen:cljs-1762#diff-84ffd22349e5ca1fe6322cb0e379b3b1R1530

Comment by Juho Teperi [ 27/Sep/16 5:15 PM ]

Proposed patch. Contains several FIXME notes, some I know how to fix, on others I would like to have some comments what would be best way to solve them.

Comment by David Nolen [ 28/Sep/16 8:20 AM ]

This is fantastic. Will review!

Comment by Juho Teperi [ 12/Oct/16 2:34 PM ]

Went through my fixme notes and added measure call around module processing.

Comment by David Nolen [ 18/Oct/16 4:40 PM ]

fixed https://github.com/clojure/clojurescript/commit/732034bc8cb7e356b211c5d2b88a7af94ba184e5





[CLJS-1750] With `:language-out :ecmascript5-strict` goog.global is undefined Created: 15/Aug/16  Updated: 16/Jun/17  Resolved: 16/Sep/16

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.76
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Dusan Maliarik Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: closure


 Description   

When using `:language-out :ecmascript5-strict` the compiled code ends up in a wrapper where `this` resolves to `undefined`. This break goog/base.js where `this` is assigned to `goog.global`.



 Comments   
Comment by David Nolen [ 15/Aug/16 2:34 PM ]

This doesn't seem like a bug but rather a faithful interpretation of strict mode - see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode.





[CLJS-1682] :foreign-libs with module conversion does not works properly if it is used form deps.cljs Created: 13/Jun/16  Updated: 23/Sep/16  Resolved: 23/Sep/16

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.36, 1.9.76
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Andrey Antukh Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: closure, compiler, foreign-libs
Environment:

Linux, openjdk8


Attachments: Text File CLJS-1682.patch    
Patch: Code

 Description   

When :foreign-libs is used for consume commonjs (or amd) modules from clojurescript using the `deps.cljs` mechanism, an unexpected "internal compiler error" is raised. When the same configuration is attached on the build script, everything works as expected.

Simple way to reproduce that, create sample directory tree:

mkdir tmp;
cd tmp;
mkdir -p src/testapp
mkdir -p src/vendor
touch src/testapp/core.cljs
touch src/vendor/greeter.js

Download the latest compiler or copy the recently build one from master:

wget https://github.com/clojure/clojurescript/releases/download/r1.9.36/cljs.jar

Create the sample cljs file:

;; tmp/src/testapp/core.cljs
(ns testapp.core
  (:require [cljs.nodejs :as nodejs]
            [greeter.core :as g]))

(nodejs/enable-util-print!)

(defn -main
  [& args]
  (println (g/sayHello "Ciri")))

(set! *main-cli-fn* -main)

Create the sample commonjs module:

"use strict";

exports.sayHello = function(name) {
  return `Hello ${name}!`;
};

Create the build script (that works):

;; tmp/build.clj
(require '[cljs.build.api :as b])

(b/build "src"
 {:main 'testapp.core
  :output-to "out/main.js"
  :output-dir "out"
  :target :nodejs
  :language-in  :ecmascript6
  :language-out :ecmascript5
  :foreign-libs [{:file "vendor/greeter.js"
                  :module-type :commonjs
                  :provides ["greeter.core"]}]
  :verbose true})

And compile this using the following command:

java -cp cljs.jar:src clojure.main build.clj

This will generate successfully the final artifact that can be successufully executed with node:

node out/main.js
# => "Hello Ciri!"

But, if you remove the `:foreign-libs` from the build script and create a new `src/deps.cljs` file
with the following content:

{:foreign-libs [{:file "vendor/greeter.js"
                 :module-type :commonjs
                 :provides ["greeter.core"]}]}

And try compile it:

$ java -cp cljs.jar:src clojure.main build.clj
Copying jar:file:/home/niwi/tmp/cljs.jar!/cljs/core.cljs to out/cljs/core.cljs
Reading analysis cache for jar:file:/home/niwi/tmp/cljs.jar!/cljs/core.cljs
Compiling out/cljs/core.cljs
Using cached cljs.core out/cljs/core.cljs
Copying jar:file:/home/niwi/tmp/cljs.jar!/cljs/nodejs.cljs to out/cljs/nodejs.cljs
Compiling out/cljs/nodejs.cljs
Compiling src/testapp/core.cljs
Copying jar:file:/home/niwi/tmp/cljs.jar!/cljs/nodejs.cljs to out/cljs/nodejs.cljs
Copying jar:file:/home/niwi/tmp/cljs.jar!/cljs/nodejscli.cljs to out/cljs/nodejscli.cljs
Copying jar:file:/home/niwi/tmp/cljs.jar!/goog/base.js to out/goog/base.js
Copying jar:file:/home/niwi/tmp/cljs.jar!/goog/string/string.js to out/goog/string/string.js
Copying jar:file:/home/niwi/tmp/cljs.jar!/goog/object/object.js to out/goog/object/object.js
Copying jar:file:/home/niwi/tmp/cljs.jar!/goog/string/stringbuffer.js to out/goog/string/stringbuffer.js
Copying jar:file:/home/niwi/tmp/cljs.jar!/goog/debug/error.js to out/goog/debug/error.js
Copying jar:file:/home/niwi/tmp/cljs.jar!/goog/dom/nodetype.js to out/goog/dom/nodetype.js
Copying jar:file:/home/niwi/tmp/cljs.jar!/goog/asserts/asserts.js to out/goog/asserts/asserts.js
Copying jar:file:/home/niwi/tmp/cljs.jar!/goog/array/array.js to out/goog/array/array.js
Copying file:/home/niwi/tmp/src/vendor/greeter.js to out/greeter.js
Exception in thread "main" java.lang.RuntimeException: INTERNAL COMPILER ERROR.
Please report this problem.

null
  Node(SCRIPT): vendor/greeter.js:1:0
[source unknown]
  Parent: NULL, compiling:(/home/niwi/tmp/build.clj:3:1)
        at clojure.lang.Compiler.load(Compiler.java:7391)
        at clojure.lang.Compiler.loadFile(Compiler.java:7317)
        at clojure.main$load_script.invokeStatic(main.clj:275)
        at clojure.main$script_opt.invokeStatic(main.clj:335)
        at clojure.main$script_opt.invoke(main.clj:330)
        at clojure.main$main.invokeStatic(main.clj:421)
        at clojure.main$main.doInvoke(main.clj:384)
        at clojure.lang.RestFn.invoke(RestFn.java:408)
        at clojure.lang.Var.invoke(Var.java:379)
        at clojure.lang.AFn.applyToHelper(AFn.java:154)
        at clojure.lang.Var.applyTo(Var.java:700)
        at clojure.main.main(main.java:37)
Caused by: java.lang.RuntimeException: INTERNAL COMPILER ERROR.
Please report this problem.

[...]


 Comments   
Comment by Andrey Antukh [ 13/Jun/16 5:42 AM ]

Also happens with `cljs.jar` build from master.

Comment by Rohit Aggarwal [ 14/Jun/16 5:40 AM ]

[Compiler noob here]

Here is what is causing the issue:

In src/main/clojure/cljs/closure.clj in process-js-modules function, in the first case :foreign-libs is being set in opts and in the second failing case :ups-foreign-libs is being set in opts.

I am investigating the root of this.

Comment by Rohit Aggarwal [ 14/Jun/16 6:11 AM ]

A fix is to that set foreign-libs keyword in opts to a union of both foreign-libs and ups-foreign-libs.

I've verified that it works for both the above given examples. But I don't know enough about the compiler to propose this change.

Comment by Rohit Aggarwal [ 14/Jun/16 10:57 AM ]

Attaching patch with fixes this problem. The patch keeps the two sets of data (ups-foreign-libs, foreign-libs) separate.

I've run all the tests and they pass.

Comment by David Nolen [ 23/Sep/16 2:31 PM ]

fixed https://github.com/clojure/clojurescript/commit/eaebc1cdb39e3fed9e7e36ec1315337b77a0350d





[CLJS-1036] cljs.closure/build does not find upstream dependencies when called from worker thread Created: 13/Feb/15  Updated: 20/Feb/15  Resolved: 20/Feb/15

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Michael Griffiths Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: cljs, closure, compiler

Attachments: Text File CLJS-1036--001.patch    

 Description   

Example stacktrace: https://www.refheap.com/97198 (context is using figwheel in a clojure REPL from CIDER)

Because cljs.closure/build calls the 0-arity form of get-upstream-deps, it is implicitly using the current thread's classloader to find the deps.cljs resources. It is then assoc'ing the result into opts, so the caller has no way of gathering the dependencies themselves and passing them in.



 Comments   
Comment by Michael Griffiths [ 13/Feb/15 1:40 PM ]

Patch CLJS-1036--001.patch, attached, uses merge instead of assoc to allow passing of the upstream dependency data via opts.

Comment by Michael Griffiths [ 13/Feb/15 1:45 PM ]

We could also allow passing of a :resources-classloader opt so callers don't have to gather the dependencies themselves, if you'd prefer a patch that does that.

Comment by Bruce Hauman [ 15/Feb/15 2:17 PM ]

This is a problem with calling cljs.closure/build inside nREPL in general when upstream deps are involved.

Comment by David Nolen [ 15/Feb/15 2:34 PM ]

I would like Chas Emerick to chime in on this thread before considering it.

Comment by Chas Emerick [ 16/Feb/15 9:02 AM ]

Could someone point me to a simple (sample?) project that uses upstream dependencies? I haven't used them yet, and don't have such a thing handy.

The proposed patch is confusing to me; if the problem is in resolving dependencies due to classloader state/configuration, how does it help to give opts the opportunity to clobber what get-upstream-deps returns? That is, doesn't this just move the problem downstream to who/whatever is calling build?

Comment by Michael Griffiths [ 16/Feb/15 10:10 AM ]

Chas: yes, the proposed patch simply moves the responsibility to the caller. I was under the assumption that changing how get-upstream-deps/build resolves the classloader by default would be too-breaking of a change, and had not even considered the fact that this might be a broader issue with nREPL itself.

Example usage of get-upstream-deps in the wild (that even mentions classloader issues): https://github.com/immutant/immutant-release/blob/ea538feb548bde86ebce20ec679da7a19b639259/integration-tests/apps/ring/basic-ring/src/basic_ring/core.clj#L51

Note that Weasel (at least) is currently gathering the upstream deps itself too: https://github.com/tomjakubowski/weasel/commit/5cd7009f584100f0d89fc7f00079458bb1f2c016

I'll set up an example project tonight if you like, but I think all you need is to add [cljsjs/react "0.12.2-5"] as a dependency to an existing cljs project and then require [cljsjs.react] in one of its namespaces.

Comment by Bruce Hauman [ 16/Feb/15 1:04 PM ]

Chas, David here is a very quick example:

If you do a

lein new om-mies hello-world
cd hello-world
lein repl
user> (require '[cljs.closure :as cc])
user> (cc/build "src" {})

Assuming lein repl started an nREPL repl. This will produce the error mentioned above. The problem is that om is utilizing the new deps.js facility to autmatically import cljsjs.react. get-upstream-deps depends on being run on the main thread.

https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/closure.clj#L1116

A solution that fixes this in get-upstream-deps* would be preferable.

Is this simple solution to use the system classloader when not on the main thread:

(if (not= (.getName (Thread/currentThread)) "main")
  (java.lang.ClassLoader/getSystemClassLoader)
  (. (Thread/currentThread) (getContextClassLoader)))

To naive? My experience with Java threads and classloaders is minimal.

Or should we always use the (java.lang.ClassLoader/getSystemClassLoader) in this case?

Comment by Chas Emerick [ 17/Feb/15 3:35 PM ]

Using the system classloader isn't necessary. get-upstream-deps is currently using .findResources, not .getResources; the difference is that the former looks only for matching resources in the target classloader, while the latter first delegates to the parent classloader, and only looks locally if nothing is found there. So, just using .getResources instead will make all current usage work as expected, I think. e.g.

;; `lein repl` nREPL here
user=> (enumeration-seq (.getResources (. (Thread/currentThread) (getContextClassLoader))  "deps.cljs"))
(#<URL jar:file:/home/chas/.m2/repository/cljsjs/react/0.12.2-5/react-0.12.2-5.jar!/deps.cljs>)
user=> (enumeration-seq (.findResources (. (Thread/currentThread) (getContextClassLoader))  "deps.cljs"))
nil

There are some classloader edge cases that might motivate making get-upstream-deps perform a more comprehensive search, explicitly pinging all classloaders for matching resources. This is sometimes necessary for these sorts of scanning scenarios if e.g. a deps.cljs file is available via a parent classloader, which will therefore "shadow" other deps.cljs files in child classloaders. In that case, you need something like cemerick.pomegranate/resources.

Hope that helps.

Comment by David Nolen [ 17/Feb/15 3:46 PM ]

Chas's suggested fix has been committed to master. It works on the simple tests I've run but I'd like to hear more confirmation. Thanks everyone.

https://github.com/clojure/clojurescript/commit/79208f5bf15825a2ba2d0ce95aae6d2b71966494

Comment by David Nolen [ 20/Feb/15 4:36 PM ]

Closing this one. Chas's solution is the correct one.





[CLJS-161] Update bootstrap to current version of Google's Closure Library Created: 14/Mar/12  Updated: 27/Jul/13  Resolved: 08/May/12

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Task Priority: Major
Reporter: Gianni Chiappetta Assignee: Hubert Iwaniuk
Resolution: Completed Votes: 0
Labels: bootstrap, closure
Environment:

N/A


Attachments: Text File CLJS-161-CLJS-35-with-static-serving.patch    

 Description   

The current bundled version of Google's Closure Library is out of date and missing a lot of useful additions. The bootstrap currently requests version r790, however r1376 has been available for several months now.

The latest version can be found here: http://closure-library.googlecode.com/files/closure-library-20111110-r1376.zip



 Comments   
Comment by David Nolen [ 14/Mar/12 7:09 PM ]

CLJS-35 has a non-working patch. If it's fixed for OS X, I'll apply it.

Comment by Gianni Chiappetta [ 15/Mar/12 10:44 AM ]

@David Once the patch is fixed, can we do both?

Comment by David Nolen [ 15/Mar/12 11:12 AM ]

The ideal patch would allow you to get head, a specific revision. Unless I'm missing something it would also be nice to get a bootstrap.bat

Comment by Hubert Iwaniuk [ 21/Apr/12 3:17 AM ]

Attached patch uses newer version of GClosure.

Comment by David Nolen [ 21/Apr/12 9:27 AM ]

fixed, https://github.com/clojure/clojurescript/commit/8b74d8dcb4edeb80fda72ae7f7c1e5872dc59687

Comment by David Nolen [ 22/Apr/12 3:34 PM ]

Reverted. This patch related to CLJS-35 broke browser REPL. I'll happily apply a new patch that addresses the browser REPL issues

Comment by Hubert Iwaniuk [ 30/Apr/12 3:37 AM ]

New GClosure and static serving.

Comment by Hubert Iwaniuk [ 08/May/12 6:05 AM ]

New patch should solve both, this and CLJS-35.

Comment by David Nolen [ 08/May/12 11:38 PM ]

fixed, https://github.com/clojure/clojurescript/commit/63eea77d4d6a2ec31bb017da9f343be5f29a61a4





Generated at Mon Sep 25 13:51:02 CDT 2017 using JIRA 4.4#649-r158309.