ESM files used in a shadow-cljs project have its property-renamed under advanced optimization

[lins05]

Issue Decription

ESM files used in a shadow-cljs project have its property-renamed.

Ofc this is fix-able by using an externs file. But it's cumbersome to add lots of externs. Ideally this shall work out of the box without any externs files.

Repro

// js/app/util_esm.js

export function callDogBark(dog, n) {
  // The for loop is to prevent the function from being inlined.  
  for (let i = 0; i < n; i++) {
    dog.bark();
  }
}

;; src/app/main.cljs
(ns app.main
  (:require ["/app/util_esm.js" :as ESM]))

(ESM/callDogBark #js {:bark (fn []
                              (println "Bark from ESM!"))} 1)

Dev build runs fine:

$ yarn node -e "require('./output/dev/app.main.js')"

Bark from ESM!

Relese build fails due to the renaming:

$ yarn node -e "require('./output/release/app.main.js')"

my-repros/repro-advanced-renaming-js/output/release/app.main.js:46
  $dog$jscomp$inline_477$$.$bark$();
                           ^

TypeError: $dog$jscomp$inline_477$$.$bark$ is not a function
    at Object.<anonymous> (my-repros/repro-advanced-renaming-js/output/release/app.main.js:46:28)

From the output we can tell that bark is renamed to $bark$. If we turn off :pseudo-names true it would be renamed to shorter names like Nb.

I've created a simple repro project in this repo for easier local reproduction.

[thheller]

It is fully intended that the properties get renamed. Since it goes through the same :advanced optimizations as all CLJS and other Closure code in can get all the benefits as well. If everything is renamed in the same step, everything should still work.

The problem in a nutshell however is that #js {:bark ...} generates this JS {"bark": ...}, so just an object with a string key. Unfortunately this breaks one of the Closure Compiler rules about not mixing string and property access. Thus .bark gets renamed but {"bark" ...} doesn't since string keys are never touched.

So, the problem here isn't so much that bark is getting renamed, it is that CLJS code can't create the proper Object needed here easily.

Well, you could write (reify Object (bark [this] ...)) or a full deftype. (doto #js {} (set! -bark (fn [] ...)) also works. None of them ideal, but the best we can do right now.

Unfortunately there is no toggle in the Closure Compiler to tell it to not rename properties in certain files. It is either all or nothing. If you prefer nothing you can rewrite the file as CommonJS. CommonJS is processed like node_modules files and only go through :simple. Here however you have the same problem in reverse, reify/deftype woudln't work.

It sucks, but I have no easy solution other then turning off renaming completely. Or maybe collecting all property names in JS files and automatically adding them to externs, which still would make builds larger than they probably should be.

[lins05]

Thanks for the reply! In the GCC source I think the relevant code for props renaming is here: https://github.com/google/closure-compiler/blob/v20230103/src/com/google/javascript/jscomp/RenameProperties.java#L292-L374 , and as you mentioned, it doesn't expose any interface to skip a single file.

Looks like the only way to have the good of both worlds (esm & advanced optization) is to modify this file, and place it in the project classpath to shadow the original implementation.

[lins05]

Another issue since we're here: When doing release build, GCC would warn about the ESM module variable is undeclared, which could be confusing:

> clj -A:shadow-cljs -M:shadow-cljs "release" "app"

[:app] Compiling ...
------ WARNING #1 -  -----------------------------------------------------------
 File: my-repros/repro-shadow-cljs-advanced-renaming-esm/src/app/main.cljs:4
--------------------------------------------------------------------------------
   1 | (ns app.main
   2 |   (:require ["/app/util_esm.js" :as ESM]))
   3 |
   4 | (ESM/callDogBark #js {:bark (fn []
--------------------------------------------------------------------------------
 variable module$app$util_esm is undeclared
--------------------------------------------------------------------------------
   5 |                               (println "Bark from ESM!"))} 1)
   6 |
--------------------------------------------------------------------------------
nil
[:app] Build completed. (41 files, 1 compiled, 0 warnings, 10.84s)

Any idea how to hide these?

[thheller]

Hmm never seen this. I guess is that it is :npm-module related. :npm-module is already rather hacky, and adding the JS support makes it a bit more sketchy I guess. I don't really recommend using :npm-module, :esm might be a better choice if thats an option.

[lins05]

I just tried :esm target but looks like in dev builds the output is problematic:

build config:

  :app-esm {:target :esm
            :modules {:main {:exports {go app.main/go}}}
            :dev {:output-dir "output/dev-esm"}
            :release {:output-dir "output/release-esm"
                      :compiler-options {:optimizations :advanced
                                         :pretty-print true
                                         :pseudo-names true}
                     }}

cljs source

;; src/app/main.cljs
(ns app.main
  (:require ["/app/util_esm.js" :as ESM]))

(ESM/callDogBark #js {:bark (fn []
                                (println "Bark from ESM!"))} 1)


(defn go []
  (js/console.log "Go ESM!"))

Dev build output:

// output/dev-esm/cljs-runtime/app.main.js
import "./cljs_env.js";
goog.provide("app.main");
module$app$util_esm.callDogBark(
  {
    bark: function () {
      return cljs.core.println.cljs$core$IFn$_invoke$arity$variadic(
        cljs.core.prim_seq.cljs$core$IFn$_invoke$arity$2(["Bark from ESM!"], 0)
      );
    },
  },
  1
);
app.main.go = function app$main$go() {
  return console.log("Go ESM!");
};

Apparently the variable module$app$util_esm is not defined in the app.main.js file. The repro git repo is also updated with this use case.

However the release build is fine though .

[lins05]

I think I somehow get it.

The :esm target is different from :npm-module that the latter has a :ns-regexp option as a syntax sugar to collect a list of modules using the regex, while for :esm one must provide the modules manually.

So files under output/dev-esm/cljs-runtime/ themselves are not valid esm modules, the only valid output esm file are output/dev-esm/main.js.

I think it would be useful to support :ns-regexp in :esm target as well, e.g. the js output of all cljs files that mathces -test$ could be handled by a javascript land testing framework like cypress, which is my current use case.

[thheller]

Yes, for dev builds the "ESM" is a bit hacked together. So that REPL and hot-reload still work. release builds looks entirely different.

Are you saying that release also doesn't work? For dev the module$app$util_esm it should exist as a global.