shadow-cljs icon indicating copy to clipboard operation
shadow-cljs copied to clipboard

Compilation issue in shadow-cljs >= 2.17.0

Open knubie opened this issue 1 year ago • 6 comments

I tried upgrading my project from shadow-cljs 2.14.0 to the latest version recently, but the build is throwing this error:

[2022-07-09 15:32:00.212 - WARNING] :shadow.cljs.devtools.server.util/handle-ex - {:msg {:type :start-autobuild}}
AssertionError Assert failed: (str/starts-with? rel-require "./")
	shadow.build.npm/find-resource-in-package (npm.clj:674)
	shadow.build.npm/find-resource-in-package (npm.clj:674)
	shadow.build.npm/find-resource (npm.clj:779)
	shadow.build.npm/find-resource (npm.clj:724)
	shadow.build.resolve/find-npm-resource (resolve.clj:122)
	shadow.build.resolve/find-npm-resource (resolve.clj:93)
	shadow.build.resolve/fn--11775 (resolve.clj:262)
	shadow.build.resolve/fn--11775 (resolve.clj:230)
	clojure.lang.MultiFn.invoke (MultiFn.java:244)
	shadow.build.resolve/find-resource-for-string (resolve.clj:80)
	shadow.build.resolve/find-resource-for-string (resolve.clj:69)
	shadow.build.resolve/resolve-string-require (resolve.clj:453)

Looks like there was some changes introduced in ed908d820f5571ec5544f15aabd622cfca5b4f4e that validates requires in npm packages somehow? Problem is I don't know which package is causing the issue. (I tested 2.17.0 vs 2.16.12 to confirm that 2.17.0 is the version that breaks the build.)

Do you have any tips or pointers as to how I can track down the cause of the failure?

knubie avatar Jul 09 '22 19:07 knubie

Uhm I don't have any guesses as to what might be causing this.

I guess to debug this you can open shadow-cljs clj-repl and then eval this

(alter-var-root #'shadow.build.npm/find-resource-in-package
  (fn [orig]
    (fn [npm package require-from rel-require]
      (try
        (orig npm package require-from rel-require)
        (catch Exception e
          (tap> [:what rel-require package require-from])
          (throw e))))))

Then run a normal compilation (shadow/compile :your-build-id) and the tap> should provide more insight. you can look at it in the UI (defaults to http://localhost:9630/inspect).

I'd need to know what rel-require actually was and which package its from. require-from may also be relevant.

thheller avatar Jul 13 '22 14:07 thheller

Thanks for the tip @thheller !

I've tracked down the issue to this package: https://github.com/knubie/remarkable-brackets

and the value of rel-require is ".". Could this file be the culprit? https://github.com/knubie/remarkable-brackets/blob/master/test/test.js Notice the require that starts with ../.

knubie avatar Jul 14 '22 00:07 knubie

I would assume that the test.js file isn't actually required when used in a build? But ../ is also a perfectly fine require. Just "." isn't but I'm unsure where it gets that from. How do you require it from CLJS? Maybe I can replicate it somehow. Is this package on npm?

thheller avatar Jul 14 '22 05:07 thheller

Yeah it shouldn't be required; I was just taking a wild guess there because I couldn't find anything that looked like ".".

That package is not on NPM, I'm declaring it as a dependency in my package.json like this:

{
  // ...
  "devDependencies": {
    // ...
    "remarkable-brackets": "git+https://github.com/knubie/remarkable-brackets.git",
  }
}

And requiring it in cljs like this:

(ns memo.markdown
  (:require
    ;; ...
    ["remarkable-brackets" :as rm-brackets]))

;; Using it like this:
(rm-brackets #js{...})

The repo is mostly a fork of https://gitlab.com/msteedman/remarkable-furigana (which I'm also requiring in the package.json and cljs), which is itself a fork of https://gitlab.com/jordanr/remarkable-furigana which is on NPM.

remarkable-furigana might have the same issue, and I tried removing my require to remarkable-brackets to check if it did, but I started to get a different error:

Execution error (StackOverflowError) at shadow.build.npm/service? (npm.clj:118).

Which I guess we can save for later.

knubie avatar Jul 14 '22 13:07 knubie

Found the cause I guess. It gets confused by the "name" field in package.json not matching the actual node_modules folder name. If you fix that in the package the problem goes away.

thheller avatar Jul 14 '22 16:07 thheller

Leaving this as a note for myself.

Since the name in package.json is different and longer this code ends up as (str "." "") calling find-require-in-package with "." when it should not.

https://github.com/thheller/shadow-cljs/blob/5461a12ac46daa7b0edc53ecae589f92769b2a81/src/main/shadow/build/npm.clj#L809

This was only supposed to shorten the require to be package specific. Can't remember why this doesn't remove everything until first /?

thheller avatar Jul 14 '22 18:07 thheller

Found the cause I guess. It gets confused by the "name" field in package.json not matching the actual node_modules folder name. If you fix that in the package the problem goes away.

Yep, you're absolutely right. 😅 Sorry for the incredibly late follow up. I'll close this issue now.

knubie avatar Aug 12 '22 14:08 knubie