Skip to content
This repository has been archived by the owner on Jun 4, 2022. It is now read-only.

Upgrade to ClojureScript 1.10 w/o Google Closure Compiler #379

Merged
merged 16 commits into from May 7, 2018

Conversation

arichiardi
Copy link
Collaborator

@arichiardi arichiardi commented Mar 24, 2018

For now this is what has been done:

  • Embed our own build-affecting-options.
  • Apply all upstream changes not involving the Google Closure Compiler
  • Bundle cljs.core.specs.alpha
  • Use cljs.core/eval in lumo.core

@arichiardi
Copy link
Collaborator Author

arichiardi commented Mar 24, 2018

Unfortunately the commit google/closure-compiler@c668ed7 break us big time because it seems it does not emit goog.provide anymore for compiled outputs.

Bigger time than the JVM implemenation because we don't have access to classes and therefore we cannot hack our own solution.

I don't still grasp all the implications but is seems that

(.split #"(?=goog\.provide\(\")"))]
is not splitting properly into processed modules.

@arichiardi
Copy link
Collaborator Author

Juho pushed another nice fix there: clojure/clojurescript@b1de2f8

Keeping track here just for the sake of it, at the moment these methods are not exposed to the JS compiler.

@arichiardi
Copy link
Collaborator Author

arichiardi commented Mar 24, 2018

After a night's sleep I think the possible approaches to this are three:

  • deviate from core and get dependency info + process some other way
  • don't bump the old JS compiler with the current implementation - meh
  • have the Closure team expose some dependency info + methods

@arichiardi
Copy link
Collaborator Author

arichiardi commented Mar 26, 2018

I am trying things at the REPL and what stopped working is the following:

(def compiledCode "var ...")
(closure/parse-js-ns (.split compiledCode #"(?=goog\.provide\(\")"))

which happens here:

https://github.com/anmonteiro/lumo/blob/cljs-1.10/src/cljs/bundled/lumo/closure.cljs#L1454

@arichiardi arichiardi changed the title WIP - Upgrade to ClojureScript 1.10.X Upgrade to ClojureScript 1.10.X Mar 27, 2018
@arichiardi arichiardi force-pushed the cljs-1.10 branch 2 times, most recently from 149c45c to d6bc1d5 Compare March 28, 2018 21:04
@arichiardi
Copy link
Collaborator Author

Something I discovered today on this branch is this:

cljs.user=> (doc println)
-------------------------cljs.core/println([& objs])  Same as print followed by (newline)nil

@arichiardi
Copy link
Collaborator Author

New GCC version is out finally solving our long standing source map bug!

I will integrate it here. Wondering if after merging this we should make an alpha release. I am currently using the two outstanding patches merged together into one on node with no issues.

@arichiardi
Copy link
Collaborator Author

Rebased on master in order to have the transit fixes.

@arichiardi
Copy link
Collaborator Author

This PR is stuck because of the compiler issue, still haven't found a way to pass post GCC module processing around. Vanilla Cljs queries the Google Closure Compiler, we can't (see above).

@arichiardi
Copy link
Collaborator Author

arichiardi commented Apr 26, 2018

Got some debugging of what happens in get-closure-sources with the can-promise library (choosen because tiny):

[[node-module-deps]]
*** module type :es6
*** module type :es6
*** module type :es6
*** module type :es6
js-modules (module$home$arichiardi$git$lumo_repros$node_modules$can_promise$index module$home$arichiardi$git$lumo_repros$node_modules$window_or_global$lib$index module$home$arichiardi$git$lumo_repros$node_modules$can_promise$package_json module$home$arichiardi$git$lumo_repros$node_modules$window_or_global$package_json)
---
var module$home$arichiardi$git$lumo_repros$node_modules$can_promise$index={};var G$$module$home$arichiardi$git$lumo_repros$node_modules$can_promise$index=module$home$arichiardi$git$lumo_repros$node_modules$window_or_global$lib$index.default;
module$home$arichiardi$git$lumo_repros$node_modules$can_promise$index.default=function(){return typeof module$home$arichiardi$git$lumo_repros$node_modules$window_or_global$lib$index.default.Promise==="function"&&typeof module$home$arichiardi$git$lumo_repros$node_modules$window_or_global$lib$index.default.Promise.prototype.then==="function"};var module$home$arichiardi$git$lumo_repros$node_modules$window_or_global$lib$index={};module$home$arichiardi$git$lumo_repros$node_modules$window_or_global$lib$index.default=typeof self==="object"&&self.self===self&&self||typeof global==="object"&&global.global===global&&global||this;
---
parse-js-ns {:requires [], :provides []}
Cannot read property 'replace' of null

	evalmachine.<anonymous>/clojure.string.replace (evalmachine.<anonymous>:2104:72)
	evalmachine.<anonymous>/ (evalmachine.<anonymous>:2840:39)
	evalmachine.<anonymous>/ (evalmachine.<anonymous>:1072:205)
	evalmachine.<anonymous>/cljs.core.LazySeq.fn (evalmachine.<anonymous>:1072:210)
	evalmachine.<anonymous>/cljs.core.LazySeq.sval (evalmachine.<anonymous>:788:151)
	cljs.core/-seq [cljs.core/ISeqable] (evalmachine.<anonymous>:794:255)
	evalmachine.<anonymous>/Object.cljs.core.seq (evalmachine.<anonymous>:502:172)
	cljs.core/-reduce [cljs.core/IReduce] (evalmachine.<anonymous>:793:250)

no more goog.provide even with :whitespace

@arichiardi
Copy link
Collaborator Author

After more investigating, it seems like the options we need have not been exposed to the JS version of the compiler.

Unless someone does it they won't add them there because the GWT has been contributed and seems not really updated.

In particular one thing that is evident from the above is that we need --module-root, see google/closure-compiler-js#96.

This would not solve the problem though of identifying the modules in the GCC-ed file.

It seems like a combination of --module https://github.com/google/closure-compiler/blob/master/src/com/google/javascript/jscomp/CommandLineRunner.java#L223 and --module_wrapper would achieve what we need.

@arichiardi arichiardi force-pushed the cljs-1.10 branch 2 times, most recently from c408d4c to c83ab9a Compare April 26, 2018 20:55
@arichiardi arichiardi changed the title Upgrade to ClojureScript 1.10.X Upgrade to ClojureScript 1.10 w/o Google Closure Compiler Apr 26, 2018
@arichiardi arichiardi force-pushed the cljs-1.10 branch 5 times, most recently from 8059eaf to f0a5f91 Compare April 28, 2018 02:31
@arichiardi arichiardi force-pushed the cljs-1.10 branch 3 times, most recently from ba75e0e to 9f91a75 Compare May 5, 2018 02:03
@arichiardi
Copy link
Collaborator Author

Uncommented test-node-modules-cljs-2246 and test-deps-api-cljs-2255 which pass as well with flying colors 🌈

@@ -11,7 +11,7 @@ echo "### Compiling Macro Namespaces"

mkdir lumo-cljs\out\macros-tmp || goto :error

echo (require 'lumo.build.api 'lumo.analyzer 'lumo.cljs-deps 'lumo.classpath 'lumo.closure 'lumo.compiler 'lumo.io 'lumo.json 'lumo.util 'clojure.core.reducers 'clojure.zip 'clojure.data 'clojure.reflect 'clojure.browser.net 'clojure.browser.event 'cljs.nodejs 'cljs.pprint 'cljs.test 'cljs.analyzer.api 'cljs.spec.test.alpha) (require-macros 'lumo.repl 'lumo.util 'clojure.template 'cljs.pprint 'cljs.support 'cljs.spec.alpha 'cljs.spec.gen.alpha 'cljs.spec.test.alpha 'cljs.test 'cljs.reader 'cljs.env.macros 'cljs.analyzer.macros 'cljs.compiler.macros) | build\lumo.exe --quiet -c target -sfdk lumo-cljs/out/macros-tmp || goto :error
echo (require 'lumo.build.api 'lumo.analyzer 'lumo.cljs-deps 'lumo.classpath 'lumo.closure 'lumo.compiler 'lumo.io 'lumo.json 'lumo.util 'clojure.core.reducers 'clojure.zip 'clojure.data 'clojure.reflect 'clojure.browser.net 'clojure.browser.event 'cljs.nodejs 'cljs.pprint 'cljs.test 'cljs.analyzer.api 'cljs.spec.test.alpha 'cljs.core.specs.alpha) (require-macros 'lumo.repl 'lumo.util 'clojure.template 'cljs.pprint 'cljs.support 'cljs.spec.alpha 'cljs.spec.gen.alpha 'cljs.spec.test.alpha 'cljs.test 'cljs.reader 'cljs.env.macros 'cljs.analyzer.macros 'cljs.compiler.macros) | build\lumo.exe --quiet -c target -sfdk lumo-cljs/out/macros-tmp || goto :error
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing moving the generated files for cljs.core.specs.alpha to the target dir

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I thought I added those, will add them again

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adressed.

@@ -8,4 +9,36 @@
(when-not (cljs.test/successful? m)
(lumo/exit 1)))

(defmethod cljs.test/report [:cljs.test/default :error] [m]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does adding the code below help you debug what's going on in tests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep this is a better reporter basically, using this at work too, can take it away and add it in another PR as well if you want

Port code over and some tests for 1.10 support. Note that the Google Closure
Compiler has not been bumped because some changes are not compatible.
The new CLJS introduces changes that disable newline printing for lumo (and
planck), so we need to patch it. See planck-repl/planck#3445dd7.
arichiardi and others added 5 commits May 5, 2018 12:32
Apparently cljs/core.cljs.cache.json has been moved in the jar (and on the
fileset) and for this reason tests were failing.
@anmonteiro anmonteiro merged commit d0dfa58 into master May 7, 2018
@anmonteiro anmonteiro deleted the cljs-1.10 branch May 7, 2018 01:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants