<< Back to previous view

[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/Sep/16  Resolved: 16/Sep/16

Status: Resolved
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 Tue Dec 06 15:58:27 CST 2016 using JIRA 4.4#649-r158309.