Skip to content

Commit

Permalink
fix ESM .js code referencing other ESM .js code
Browse files Browse the repository at this point in the history
Closure assumes that the final output is all in the same scope
but for node that isn't true currently. So instead of trying to
change the way Closure rewrites we just collect the names it
creates and lift those into the global scope when needed.

only really affected watch/compile builds. release doesn't have
this problem.

fixes #836
fixes #521
  • Loading branch information
thheller committed Feb 4, 2021
1 parent 77dfff2 commit 8b7f396
Show file tree
Hide file tree
Showing 6 changed files with 83 additions and 34 deletions.
4 changes: 3 additions & 1 deletion src/dev/demo/more-es6.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,6 @@ function bar(x) {
return x;
};

export { bar };
let foo = 1;

export { bar, foo };
8 changes: 2 additions & 6 deletions src/dev/demo/script.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,9 @@
["request" :as req]
;; ["./cjs.js" :as cjs]
[demo.js-class :refer (Foo)]
["./es6.js" :as es6]
["which" :as which]))

;; (js/console.log "cjs" cjs)

(js/console.log "Foo" Foo)

(prn [:goog.global js/goog.global.setTimeout])

(req "https://www.google.com"
(fn [error res body]
(prn [:error error])
Expand All @@ -29,6 +24,7 @@

(defn main [& args]
(js/console.log "starting server")
(es6/foo "bar")
(let [server (http/createServer #(request-handler %1 %2))]

(tap> [:hello-world js/process.env])
Expand Down
46 changes: 44 additions & 2 deletions src/main/shadow/build/closure.clj
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,11 @@
(.getDeclaredField "injectedLibraries"))
(.setAccessible true)))

(def own-symbols-field
(doto (-> (Class/forName "com.google.javascript.jscomp.SymbolTable$SymbolScope")
(.getDeclaredField "ownSymbols"))
(.setAccessible true)))

;;;
;;; partially taken from cljs/closure.clj
;;; removed the things we don't need
Expand Down Expand Up @@ -1021,6 +1026,11 @@
(keys)
(set)))

(defn get-own-symbol-names [symbol-scope]
(-> (.get own-symbols-field symbol-scope)
(keys)
(set)))

(defn dump-closure-inputs [state externs js-mods compiler-options]
(doseq [extern externs]
;; externs have weird filenames and we don't need to keep folders intact
Expand Down Expand Up @@ -1526,7 +1536,15 @@
(.getSourceMap cc)

injected-libs
(get-injected-libs cc)]
(get-injected-libs cc)

global-symbol-names
(-> (.buildKnownSymbolTable cc)
(.getGlobalScope)
(get-own-symbol-names))

eval-in-global-scope
(get state ::output/eval-in-global-scope true)]

(throw-errors! state cc result)

Expand All @@ -1539,6 +1557,7 @@


(let [name (.getSourceFileName source-node)]

(cond
(= polyfill-name name)
state
Expand Down Expand Up @@ -1597,6 +1616,18 @@
js
(str shadow-js-prefix js)

;; closure creates var bar$$module$some$file
;; for export let bar = 1 in /some/file.js
;; which is fine if everything is in the same global scope
;; but for node they are in their own local scope so we need to lift them into the global scope later
sym-str
(str ns)

names-in-this-file
(->> global-symbol-names
(filter #(str/ends-with? % sym-str))
(set))

sm-json
(.toString sw)

Expand All @@ -1618,7 +1649,18 @@
;; var module$demo$reducers$profile = ...
;; FIXME: not sure if it will always emit this exact pattern
(str/includes? js (str "var " ns " =")))
(str "\n$CLJS." ns "=" ns ";")))
(str "\n$CLJS." ns "=" ns ";"))

;; actually lift the names into the global scope if needed
;; normally not required since everything is in the same scope
;; mostly node will have its own scope per file though
(when (and (= :dev mode) (not eval-in-global-scope) (seq names-in-this-file))
(str "\n"
(->> names-in-this-file
(map #(str "global." % " = " % ";"))
(str/join "\n")))))

:js-symbol-names names-in-this-file
:properties properties
:source (.getCode source-file)
:source-map-json sm-json}]
Expand Down
33 changes: 18 additions & 15 deletions src/main/shadow/build/targets/node_library.clj
Original file line number Diff line number Diff line change
@@ -1,17 +1,19 @@
(ns shadow.build.targets.node-library
(:refer-clojure :exclude (flush))
(:require [clojure.java.io :as io]
[clojure.string :as str]
[clojure.spec.alpha :as s]
[shadow.build.api :as cljs]
[shadow.build :as comp]
[shadow.build.targets.shared :as shared]
[shadow.build.config :as config]
[shadow.cljs.repl :as repl]
[shadow.build.node :as node]
[shadow.cljs.util :as util]
[shadow.build.data :as data]
[cljs.compiler :as cljs-comp]))
(:require
[clojure.java.io :as io]
[clojure.string :as str]
[clojure.spec.alpha :as s]
[cljs.compiler :as cljs-comp]
[shadow.cljs.repl :as repl]
[shadow.cljs.util :as util]
[shadow.build.api :as cljs]
[shadow.build :as comp]
[shadow.build.targets.shared :as shared]
[shadow.build.config :as config]
[shadow.build.node :as node]
[shadow.build.data :as data]
[shadow.build.output :as output]))

(s/def ::exports
(s/map-of
Expand Down Expand Up @@ -77,7 +79,7 @@
`(js/Object.defineProperty ~(-> k (name) (cljs-comp/munge))
(cljs.core/js-obj
"enumerable" true
"get" (fn [] ~v)) )))
"get" (fn [] ~v)))))
)])

requires
Expand Down Expand Up @@ -120,7 +122,7 @@
;; based on https://github.com/umdjs/umd/blob/master/templates/returnExports.js
[prepend append]
(-> wrapper-rc
(slurp )
(slurp)
(str/split #"//CLJS-HERE"))

prepend
Expand Down Expand Up @@ -180,7 +182,8 @@
(shared/inject-node-repl config)

(= :dev mode)
(shared/inject-preloads :main config)
(-> (shared/inject-preloads :main config)
(assoc ::output/eval-in-global-scope false))
)))

(defn flush [state mode config]
Expand Down
23 changes: 13 additions & 10 deletions src/main/shadow/build/targets/node_script.clj
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
(ns shadow.build.targets.node-script
(:refer-clojure :exclude (flush))
(:require [cljs.compiler :as cljs-comp]
[clojure.spec.alpha :as s]
[shadow.cljs.repl :as repl]
[shadow.build.node :as node]
[shadow.build :as comp]
[shadow.build.targets.shared :as shared]
[shadow.build.config :as config]
[shadow.build.api :as cljs]
))
(:require
[cljs.compiler :as cljs-comp]
[clojure.spec.alpha :as s]
[shadow.cljs.repl :as repl]
[shadow.build.node :as node]
[shadow.build :as comp]
[shadow.build.targets.shared :as shared]
[shadow.build.config :as config]
[shadow.build.api :as cljs]
[shadow.build.output :as output]
))

(s/def ::main shared/unquoted-qualified-symbol?)

Expand Down Expand Up @@ -41,7 +43,8 @@
(shared/inject-node-repl config)

(= :dev mode)
(shared/inject-preloads :main config)
(-> (shared/inject-preloads :main config)
(assoc ::output/eval-in-global-scope false))
)))

(defn check-main-exists! [{:keys [compiler-env node-config] :as state}]
Expand Down
3 changes: 3 additions & 0 deletions src/main/shadow/build/targets/npm_module.clj
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@
(= :node runtime)
(node/set-defaults)

(and (= :node runtime) (= :dev mode))
(assoc ::output/eval-in-global-scope false)

(and (= :dev mode) (:worker-info state))
(shared/merge-repl-defines config)
)))
Expand Down

0 comments on commit 8b7f396

Please sign in to comment.