<< Back to previous view

[CLJS-2389] Fix module processing after latest Closure changes Created: 27/Oct/17  Updated: 10/Feb/18  Resolved: 10/Feb/18

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

Type: Defect Priority: Major
Reporter: Juho Teperi Assignee: David Nolen
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File CLJS-2389-1.patch     Text File CLJS-2389-3.patch     Text File CLJS-2389-4.patch     Text File CLJS-2389-5.patch     Text File CLJS-2389-6.patch     Text File CLJS-2389-7.patch    
Approval: Accepted

 Description   

New Closure-compiler doesn't add goog.provide/require calls to processed modules:

https://github.com/google/closure-compiler/pull/2641

> ES6 and CommonJS modules no longer generate synthetic goog.require and goog.provide calls.

Currently {process-js-modules} uses {load-library}, which reads the goog.provide calls in the file, to determine the name for processed module, something like {module$absolute$path}.
Now that the file doesn't have goog.provide call, this breaks.

As the module name is based on file-path, we can determine the module name directly from filepath, Closure provides utility for this: {ModuleNames/fileToModuleName}.



 Comments   
Comment by Juho Teperi [ 27/Oct/17 5:53 AM ]

Attached patch fixes the first problem, which is updating {:js-module-index} mapping from Node name to processed module name.

Another problem is that analyzer/check-uses now fails when referring to symbols in processed modules:

ERROR in (test-module-name-substitution) (core.clj:4617)
expected: (= (compile (quote (ns my-calculator.core (:require [calculator :as calc :refer [subtract add] :rename {subtract sub}])))) (str "goog.provide('my_calculator.core');" crlf "goog.require('cljs.core');" crlf "goog.require('" (absolute-module-path "
src/test/cljs/calculator.js" true) "');" crlf))
  actual: clojure.lang.ExceptionInfo: Invalid :refer, var module$home$juho$Source$clojurescript$src$test$cljs$calculator/add does not exist

For some reason {missing-use?} doesn't work correctly.

Comment by Juho Teperi [ 06/Dec/17 12:47 PM ]

I'm looking into emitting those goof.provide/require calls from Cljs. I have provide working already and that fixes analyzer etc. But we also need require so that cljs_deps.js gets proper information for browser, but I haven't yet found a way to retrieve module requires from Closure.

Comment by Juho Teperi [ 12/Dec/17 12:20 PM ]

New patch should now emit goog.provide/require calls and tests pass, and I have tested this with Reagent and npm modules and foreign libs. Works with Closure v20171112 but v20171203 doesn't seem to process CommonJS at all, but doesn't give any errors.

One problem is that optmized build now prints bunch of these warnings:

Dec 12, 2017 8:10:28 PM com.google.javascript.jscomp.PhaseOptimizer$NamedPass process
WARNING: Skipping pass inlineTypeAliases
Dec 12, 2017 8:10:29 PM com.google.javascript.jscomp.PhaseOptimizer$NamedPass process
WARNING: Skipping pass j2clChecksPass

Reagent advanced test build size increased from 1.1M to 1.3M so it is possible (some) optimizations are not being applied.

Comment by Thomas Heller [ 12/Dec/17 12:53 PM ]

I think those warnings only appear when `:language-in` is NOT set. Setting it to `:ecmascript6` now does weird things to CLJS though which may explain your code size increase.

Comment by Juho Teperi [ 12/Dec/17 12:55 PM ]

Hmm, I looked into CompilerOptions and I thought it defaults to EcmaScript2017 (ES8_MODULES featureSet, which should enable everything), but probably I missed something.

Comment by Thomas Heller [ 12/Dec/17 4:56 PM ]

Could be that the default is too high. The warnings went away (in shadow-cljs) when setting :language-in to :ecmascript5 and I didn't look any further since that is fine for CLJS.

Comment by Juho Teperi [ 08/Jan/18 5:18 PM ]

Current patch status:

Latest v20180101 release processes CJS and ES6 OK.

The emitted JS code doesn't work, because all methods exported from CJS (and ES6 default exports) need to be accessed through default property, like:

module$path$node_modules$left_pad$index["default"](...)

module$path$node_modules$react$react["default"].cloneElement(...)

And still seeing warnings about skipping passes.

Comment by Juho Teperi [ 09/Jan/18 4:06 PM ]

Latest version modifies compiler to emit ["default"] when accessing anything in JS modules.

Still getting warnings during optimization. Setting language-in doesn't help.

Comment by Juho Teperi [ 09/Jan/18 4:34 PM ]

-4 now defaults language-in to EcmaScript 5 so all optimization passes get enabled. Optimized output size now matches 1.9.946 when tested with Reagent.

Comment by Juho Teperi [ 10/Jan/18 2:22 PM ]

-5 reads module type from Closure, adds it to new :js-namespaces compiler env property, and uses that on emit* :var to emit ["default"] for only CJS modules.

Comment by David Nolen [ 12/Jan/18 3:18 PM ]

I'm unable to get this patch working with `script/test`. Module processing fails on lodash package.json because we are parsing as JS source. I also modified `script/test` to invoke the new `clj` tool and leverage the new `deps.edn` file so we can use the unshaded Closure Compiler dependency and I observed no change - the same issue persists.

Comment by Juho Teperi [ 08/Feb/18 3:47 PM ]

Updated to february Closure release. script/test works if https://dev.clojure.org/jira/browse/CLJS-2375 has been applied first.

Pr-str test case had to be changed due to https://github.com/google/closure-compiler/commit/179b62cc4770fd6a9eb306d3cf529eb99e992026.

Comment by Juho Teperi [ 09/Feb/18 12:30 PM ]

Rebased

Comment by David Nolen [ 10/Feb/18 6:07 AM ]

fixed https://github.com/clojure/clojurescript/commit/72e2ab6e63b3341aa26abcbdd72dc291cbd0c462

Generated at Wed Apr 24 07:21:04 CDT 2019 using JIRA 4.4#649-r158309.