ClojureScript

Fix module processing after latest Closure changes

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • 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}.

  1. CLJS-2389-1.patch
    12/Dec/17 12:20 PM
    13 kB
    Juho Teperi
  2. CLJS-2389-3.patch
    09/Jan/18 4:15 PM
    16 kB
    Juho Teperi
  3. CLJS-2389-4.patch
    09/Jan/18 4:34 PM
    17 kB
    Juho Teperi
  4. CLJS-2389-5.patch
    10/Jan/18 2:22 PM
    24 kB
    Juho Teperi
  5. CLJS-2389-6.patch
    08/Feb/18 3:47 PM
    26 kB
    Juho Teperi
  6. CLJS-2389-7.patch
    09/Feb/18 12:30 PM
    26 kB
    Juho Teperi

Activity

Hide
Juho Teperi added a comment -

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.

Show
Juho Teperi added a comment - 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.
Juho Teperi made changes -
Field Original Value New Value
Attachment CLJS-2389-wip.patch [ 17450 ]
Juho Teperi made changes -
Assignee Juho Teperi [ deraen ]
Hide
Juho Teperi added a comment -

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.

Show
Juho Teperi added a comment - 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.
Hide
Juho Teperi added a comment -

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.

Show
Juho Teperi added a comment - 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.
Juho Teperi made changes -
Attachment CLJS-2389-1.patch [ 17553 ]
Hide
Thomas Heller added a comment -

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.

Show
Thomas Heller added a comment - 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.
Hide
Juho Teperi added a comment -

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

Show
Juho Teperi added a comment - Hmm, I looked into CompilerOptions and I thought it defaults to EcmaScript2017 (ES8_MODULES featureSet, which should enable everything), but probably I missed something.
Hide
Thomas Heller added a comment - - edited

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.

Show
Thomas Heller added a comment - - edited 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.
Hide
Juho Teperi added a comment - - edited

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.

Show
Juho Teperi added a comment - - edited 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.
Juho Teperi made changes -
Attachment CLJS-2389-wip-2.patch [ 17612 ]
Hide
Juho Teperi added a comment -

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

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

Show
Juho Teperi added a comment - Latest version modifies compiler to emit ["default"] when accessing anything in JS modules. Still getting warnings during optimization. Setting language-in doesn't help.
Juho Teperi made changes -
Attachment CLJS-2389-wip-3.patch [ 17616 ]
Juho Teperi made changes -
Attachment CLJS-2389-wip-3.patch [ 17616 ]
Juho Teperi made changes -
Attachment CLJS-2389-3.patch [ 17617 ]
Hide
Juho Teperi added a comment - - edited

-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.

Show
Juho Teperi added a comment - - edited -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.
Juho Teperi made changes -
Attachment CLJS-2389-4.patch [ 17618 ]
Juho Teperi made changes -
Assignee Juho Teperi [ deraen ] David Nolen [ dnolen ]
Hide
Juho Teperi added a comment -

-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.

Show
Juho Teperi added a comment - -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.
Juho Teperi made changes -
Attachment CLJS-2389-5.patch [ 17621 ]
Hide
David Nolen added a comment - - edited

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.

Show
David Nolen added a comment - - edited 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.
Hide
Juho Teperi added a comment -

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.

Show
Juho Teperi added a comment - 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.
Juho Teperi made changes -
Attachment CLJS-2389-6.patch [ 17693 ]
Juho Teperi made changes -
Attachment CLJS-2389-wip-2.patch [ 17612 ]
Juho Teperi made changes -
Attachment CLJS-2389-wip.patch [ 17450 ]
Hide
Juho Teperi added a comment -

Rebased

Show
Juho Teperi added a comment - Rebased
Juho Teperi made changes -
Attachment CLJS-2389-7.patch [ 17699 ]
David Nolen made changes -
Status Open [ 1 ] In Progress [ 3 ]
David Nolen made changes -
Approval Accepted [ 10005 ]
David Nolen made changes -
Resolution Completed [ 1 ]
Status In Progress [ 3 ] Resolved [ 5 ]
David Nolen made changes -
Status Resolved [ 5 ] Closed [ 6 ]

People

Vote (1)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: