All other files are in TS. So the web-worker option doesn't apply.

hmm?

:target :browser with :web-worker true for the module

it doesn't need to be split, it can just be one module

Thank you, now I get the following errors:

failed to load goog.debug.error.js ReferenceError: goog is not defined

Cannot use default debug loader outside of HTML documents.

{:deps true
 :nrepl {:port 7002}
 :builds {:worker {:target :browser
                   :output-dir "out/worker"
                   :modules {:worker {:entries [worker.main]
                                      :web-worker true}}}}}

It actually works. I was experimenting and had started the worker with type module, removing the type fixed it.

Thank you so much :)

I'm trying to use a node package in the worker but I get the following error:

(ns worker.main
  (:require
   ["comlink" :refer (expose)]
   ["pg" :as pg]))
The required JS dependency "core-util-is" is not available, it was required by "node_modules/stream-browserify/node_modules/readable-stream/lib/_stream_readable.js".

Dependency Trace:
	worker/main.cljs
	node_modules/pg/lib/index.js
	node_modules/pg/lib/client.js
	node_modules/pg/lib/crypto/sasl.js
	node_modules/pg/lib/crypto/utils.js
	node_modules/pg/lib/crypto/utils-legacy.js
	node_modules/crypto-browserify/index.js
	node_modules/create-hash/browser.js
	node_modules/cipher-base/index.js
	node_modules/stream-browserify/index.js
	node_modules/stream-browserify/node_modules/readable-stream/readable-browser.js
	node_modules/stream-browserify/node_modules/readable-stream/lib/_stream_readable.js

Doing a js/require worked. This ESM, CommonJS drama is such a headache in the JS world.

this has nothing to do with commonjs/esm as far as I can tell. this is purely electron and how it handles the bridging between node and the browser context. FWIW in this case if you use js/require shadow-cljs will ignore it and not try to bundle it. If you use the ns :require it will try to bundle it, but can't in this case due to it being a node package I guess. So you could also set :js-options {:keep-as-require #{"pg"}} in the build config to achieve the same result as js/require basically

i.e. keep shadow-cljs from trying to bundle it and let the runtime handle it instead

Thanks Thomas, there is a lot of jargon that I find hard to keep up with in the JS build world. When you say shadow won't bundle it and allow the runtime to handle it, you mean it has to imported elsewhere (say in JS code) for it to work at runtime?

when shadow-cljs bundles the code it resolves the JS files on disk, transpiles them and puts them into the resulting JS output it generates. with js/require or :keep-as-require it will just leave a require("that-package") in the resulting JS code, leaving the runtime to look it up and shadow-cljs doesn't look for it on disk. the runtime is then responsible for looking for the files, how it does that is dependent on the runtime specifically

i.e. it may just work (as it does in node usually), or may require extra work on the configuration or user side for the runtime to fulfill that request

e.g. in modern electron renderer/browser contexts it doesn't have access for require for security reasons and you are supposed to use a preload

https://www.electronjs.org/docs/latest/tutorial/tutorial-preload

(this comes from someone you has not used electron in any project ever and only glanced over the docs)

I believe there are still settings to just let you use require directly and ignore the security aspects, no clue though

all this is electron specific and has nothing to do with node/browsers/npm, so its rules apply not those

Yes electron renderer process doesn't allow things that can be done in the node env, but the renderer can spawn a web worker that can do "node" things like require and use node only packages like pg which is what I'm doing. I can do these things in the main process and use ipc to communicate with the renderer but at some point I'd like to also target the browser (building a db client just for context) so trying to keep a lot of logic in the renderer. Thanks again for the clear explanation of what happens when I js/require and ofcourse for shadow.

ah I didn't know about the worker thing

you maybe can then just set :js-options {:js-provider :require} in the build config, to let the node thing provide all the dependencies

node packages such as pg don't do well when bundled for the browser otherwise, since they can't possibly work

can I do this per module? because only the worker module runs in a node env. I only have the worker module for now but if I set that js-provider will it work when I add other modules that are meant to run on the browser env?

no this is a per build option, so it applies to all modules. :keep-as-require is basically equivalent though, so if you don't mind specifying the packages that shouldn't be bundled that is fine too

that's happening to both third party code like medley.core and first-party like metabase.lib.cache.js.

I should mention that the CLJS output is used as a library by some other code, so perhaps this is over-eager tree-shaking? certainly metabase.lib.cache/side-channel-cache is called from some ^:exported functions in :entries namespaces, though.

it looks on further inspection like those empty files are empty because all their functions got inlined into other places. I found the guts of side-channel-cache elsewhere. that seems sound, and I guess that's why it works in prod mode.

the symptom that put me onto this is that if I set :source-maps true in release mode then webpack (actually source-map-loader from npm) chokes on the resulting files. that might be a bug in that library rather than shadow-cljs, though. I'm looking into what a valid source map should look like, and whether these are malformed.

yes, this is due to the compiler moving things to places they are actually used and inlined. :npm-module is a hack basically, to make configuration extra easy it just creates a closure module for each namespace. that doesn't prevent the closure compiler from moving all the code out of that module though. I guess there could by some further analysis of the code to remove the empty modules, but they don't hurt usually

how does it choke on the source maps? could be that they are just generated incorrectly when they are empty? which I don't think is a case I considered 😛

{"version":3,"file":"medley.core.js","sections":[{"offset":{"line":1,"column":0},"map":{"version":3,"file":"medley.core.js","lineCount":1,"mappings":"A;","sources":[],"names":[],"x_google_ignoreList":[]}}]}

it is indeed basically empty, so I can see how that may break if the consumer expects this to not be empty (as in no sources)

lacking sources does seem to be the pain point, poking around in the source-map-loader and source-map-js library code.

section.consumer._(mapping.source); mapping.source is null which makes the .at(...) throw.

I tweaked the code of node_modules/source-map-js/lib/source-map-consumer.js line 1144 by adding if (!mapping.source) continue;. that lets it progress to the end of the build, but there's a bunch of errors revolving around attempting to open various files that really exist in the codebase, but are referenced under shadow/cljs/constants/metabase/lib/blah/blah.cljc and similar. those files don't exist at all; the target/cljs_release/shadow directory doesn't exist at all. the references appear in source maps like this:

"sections": [
  {
    ...
    "map": {
      ...  
        "sources": [
          "malli/core.cljc",
          "malli/transform.cljc",
          "metabase/domain_entities/converters.cljs",
          "cljs/core.cljs",
        "shadow/cljs/constants/metabase/domain_entities/converters.cljs"
        ],
        "sourcesContent": [
          "(ns malli.core\n ...",
          "(ns malli.transform\n ...",
          "(ns metabase.domain-entities.converters\n  ...",
          ";   Copyright (c) Rich Hickey. All rights reserved.\n; ...",
          ""
        ],
...

so perhaps the (falsy) empty string is the problem there? I haven't got a code reference for it but I bet it's doing something like sourcesContent[i] || fs.readFile(sources[i]) when there's no file to read. perhaps this too is really a bug in the source-map-js library, perhaps it's a spec violation or spec ambiguity in the shadow-cljs output.

I'll take a look at this next week. if it's a case of shadow-cljs breaking the spec, I'll try to fix that. if it's source-map-js making too many assumptions, I'll try to fix that. (but if you know where either of these problems lies I defer to you.)

shadow/cljs/constants/... are basically placeholders. the closure compiler needs a place to move things to that are used in the module and possibly shared. so I add an empty file to the compilation per module, as that was the easiest way to make the compiler do what I wanted it to. gotta love JS for treating "" as falsy I guess 😛

maybe the easiest fix is to just make not those files empty, maybe just // generated or something

that seems like a simple enough fix, yeah.

what is the command to trigger the failing build part?

WEBPACK_BUNDLE=production NODE_OPTIONS=--max-old-space-size=8196 webpack --bail I assume?

yarn build-release will build the CLJS (success) then the JS (errors). you can also run those parts separately with yarn build-release:cljs and yarn build-release:js.

/mnt/c/Users/thheller/code/oss/metabase/node_modules/source-map-js/lib/array-set.js:109
  throw new Error('No element indexed by ' + aIdx);
        ^

Error: No element indexed by null
    at ArraySet.ArraySet_at [as at] (/mnt/c/Users/thheller/code/oss/metabase/node_modules/source-map-js/lib/array-set.js:109:9)
    at IndexedSourceMapConsumer.IndexedSourceMapConsumer_parseMappings [as _parseMappings] (/mnt/c/Users/thheller/code/oss/metabase/node_modules/source-map-js/lib/source-map-consumer.js:1144:48)

that is the error I assume?

yep, that's the original one.

which map is the above btw? which file name?

good error reporting on the JS side. doens't even tell you which file it failed on 😞

it varies based on the exactly order. (I put some console.logs in source-map-js to find out.) any of the nearly-empty files in the release build seems to show this problem. see medley.core.js is one, metabase.lib.cache.js is another.

ok I fixed the first problem. just getting the // generated in there turns out to be difficult. I'm giving it to the closure compiler, but it doesn't seem to use it. need to figure out why

there are so many things I regret about :npm-module. it is such a hack 😛

that one is more arguably a bug at the other end, if the empty string is not supposed to be falsy.

hm, I tried to run with our deps.edn aimed at the patch you made on master but I'm getting an exception

Caused by: java.lang.ClassNotFoundException: com.google.javascript.jscomp.ShadowCompiler

I can make a release. wanted to look into the thing for a bit some more but got side tracked

if you are trying to use a :local/root version of shadow-cljs you must run lein javac first. there are some java sources that need to be compiled

I was trying :git/url + :git/sha but that may have the same issue. I'll try that locally.

dunno how you'd do that for a git url

no problem making a release if that helps

yep, that did it. now I can repro the second issue with the contents being "" and I'll see about patching the JS library.

gotcha! yep, there was an if (content) { ... } that was wrongly failing for an empty sourceContent of "". I'll send that as a patch to source-map-js and see what happens.

just found that a hacky workaround I did for some closure compiler code is no longer necessary, so maybe time to clean all this up 😛

sweet, it's working in a local release-mode JAR of Metabase, so presumably it'll work on our internal cloud build etc.

a release would be handy since I can't check in the :local/root thing.

gimme a sec. I removed the hack that indeed is no longer necessary, but the source map source is still empty even though I'm giving it // generated as the source

could you use a non-comment but empty expression instead?

though I guess Closure compiler would remove that too, unless it had some real side effect that couldn't be ignored.

but no worries in the end - I think the if ("") thing is a bug in the source map consumer library that needs patching. we can revisit some hack to make it not "" if they're really resistant to the patch for some reason.

I tried that, but the code that ends up in the source map is still empty. maybe I'm missing something, been a while since I wrote all this and it was never pretty

yeah I'm confused. I can add a literal console.log to the source and it ends up in the final output but not in the source map

try 2.26.3. in theory the source maps shouldn't contain empty sources as I never give and to the closure compiler, but for me they are still empty regardless. maybe its my setup and works for you 😛

isn't the closure compiler emptying some of these files by inlining?

yes, but source maps use the sources not the optimized output. so that shouldn't matter.

if you are curious what is sent to the closure compiler you can set :compiler-options {:dump-closure-inputs true} in the build config

that'll write all files to disk in .shadow-cljs/builds//release/closure-inputs/, looks all good but doesn't seem to matter

I'm struggling to visualize this properly, but what about this scenario: • my.lib.empty is the one that got inlined away. • since it's empty, it now has no .map file with your patch. • some.other.code depends on it, therefore there's a my.lib.empty entry in the sources and sourceContents arrays of the source map for some.other.code. • they end up empty for not super clear reasons.

correct, thats not what I'm talking about

I mean those shadow/cljs/constants/... files. the placeholder files that are referenced in the source maps

right.

they are technically empty, but https://github.com/thheller/shadow-cljs/commit/63b7276c59a9cea1aa252a907c67fec278547cf3#diff-d69e4990f5077bca0f189ec17a7559477678c1aa4e0b4054e4c4d32eb10dca1dR754 makes them not empty

at least as far as the input is concerned

during compilation https://github.com/thheller/shadow-cljs/blob/63b7276c59a9cea1aa252a907c67fec278547cf3/src/main/shadow/build/closure/ReplaceCLJSConstants.java moves constants (i.e. keywords) to those placeholder locations, so they become actual constants

but that is not part of the input. source maps should be using the inputs, but for some reason for those files don't

even when I added console.log("yo") it did not change the source maps for some reason, but the log appeared when running the output, so it was not removed

dunno, likely something obvious but I can't figure it out

2.26.3 works nicely with the patched source-map-js. with the patch reverted (ie. vulnerable to empty sourceContents) it fails as before.

shot in the dark: perhaps shadow-cljs is trying to read those "constants" files somewhere but failing and getting nil; then it gets output with (str contents) or similar producing ""?

no, never reading it

source map generation for release builds is handled pretty much by closure alone

I mean the last ditch workaround would be to just parse the JSON, replace empty sourceFileContents and write it again

but I'm trying to get rid of ugly hacks not adding them 😛

yup. and that can be Metabase's problem, writing a tiny inline webpack plugin for this. I think browsers are fine with these source maps; it's not shadow-cljs's problem that this JS library has a bug, even if the shadow-cljs output is a bit idiosyncratic. any optimizing compiler from another language to JS would be at risk of this kind of thing, and source maps are supposed to support that.

yeah browsers never had an issue with this as far as I can tell. I mean they are empty, so there is nothing to map 😛

thanks again for your prompt help with debugging and fixing these issues!

I would just remove them altogether but rewriting the entire source map is a bit overkill