ClojureScript

Regression on scoped npm-modules

Details

  • Type: Defect Defect
  • Status: In Progress In Progress
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: 1.10.238
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Approval:
    Vetted

Description

I have regression in npm-deps resolution in 1.10.x for material-components-web. For sub-dependencies it doesn't resolve the module path correctly. When I require eg. @material/snackbar it fails with Uncaught Error: Undefined nameToPath for module$$material$base$component.

Steps to reproduce:

yarn add "@cljs-oss/module-deps" "@material/snackbar"

cat <<EOF > deps.edn
{:deps {org.clojure/clojurescript {:mvn/version "1.10.145"}}}
EOF

clj -m cljs.main -d out -e "(require '[\"@material/snackbar\"])"

Resolution worked in 1.9.946

Activity

Hide
David Nolen added a comment - - edited

This ticket just needs more information. A good first step in this report would be a git bisect. Then someone needs to determine wether this is because of ClojureScript or Google Closure. If the later there's not much we can do.

Show
David Nolen added a comment - - edited This ticket just needs more information. A good first step in this report would be a git bisect. Then someone needs to determine wether this is because of ClojureScript or Google Closure. If the later there's not much we can do.
Hide
Aleš Roubíček added a comment -

I can reproduce the regression even on 1.10.63, to narrow the window. I'll prepare backwards compatible repro and bisect after that.

Show
Aleš Roubíček added a comment - I can reproduce the regression even on 1.10.63, to narrow the window. I'll prepare backwards compatible repro and bisect after that.
Hide
Aleš Roubíček added a comment - - edited

Bisected it and it was introduced in #CLJS-2389 (GCC update)

Show
Aleš Roubíček added a comment - - edited Bisected it and it was introduced in #CLJS-2389 (GCC update)
Hide
Aleš Roubíček added a comment -

Code for reproduction

Show
Aleš Roubíček added a comment - Code for reproduction
Hide
Aleš Roubíček added a comment -

When head -2 out/node_modules/@material/snackbar/index.js ends with goog.require("module$$material$base$index"); it is the broken case.

Show
Aleš Roubíček added a comment - When head -2 out/node_modules/@material/snackbar/index.js ends with goog.require("module$$material$base$index"); it is the broken case.
Hide
David Nolen added a comment -

I can reproduce and see that the dependency index file `cljs_deps.js` does not look right, so not surprised it's not working.

Show
David Nolen added a comment - I can reproduce and see that the dependency index file `cljs_deps.js` does not look right, so not surprised it's not working.
Hide
David Nolen added a comment -

Digging in further this may be a Closure issue and we'll have to wait for an upstream fix.

Show
David Nolen added a comment - Digging in further this may be a Closure issue and we'll have to wait for an upstream fix.
Hide
David Nolen added a comment -

This is a Closure Compiler issue. We need to submit a patch and then make the necessary changes for the next Closure release, this change https://github.com/swannodette/closure-compiler/commit/58012d3f1068aa588a47dc34ec6f39413aa59e62 fixes the module name issue for me.

Show
David Nolen added a comment - This is a Closure Compiler issue. We need to submit a patch and then make the necessary changes for the next Closure release, this change https://github.com/swannodette/closure-compiler/commit/58012d3f1068aa588a47dc34ec6f39413aa59e62 fixes the module name issue for me.
Hide
Aleš Roubíček added a comment -

Excellent, thank you David.

Show
Aleš Roubíček added a comment - Excellent, thank you David.
Hide
David Nolen added a comment -
Show
David Nolen added a comment - PR Closure Compiler https://github.com/google/closure-compiler/pull/2847
Hide
David Nolen added a comment -

Can we get a non-trivial expression added to this ticket that should work after the require?

Show
David Nolen added a comment - Can we get a non-trivial expression added to this ticket that should work after the require?
Hide
David Nolen added a comment -

I figured out how to test this - it does in fact seemed resolved with master + my Closure Compiler PR.

Show
David Nolen added a comment - I figured out how to test this - it does in fact seemed resolved with master + my Closure Compiler PR.
Hide
Aleš Roubíček added a comment -

Sorry, I didn't responded earlier, I had Code Retreat yesterday. It is DOM component, so the non-trivial example needs to run in browser REPL with modified index.html. I think, if emitted goog.require is correct, everything else will work fine. Thank you very much for the fix. I hope your GCC PR will be accepted soon. 1.10 looks very solid and fast.

Show
Aleš Roubíček added a comment - Sorry, I didn't responded earlier, I had Code Retreat yesterday. It is DOM component, so the non-trivial example needs to run in browser REPL with modified index.html. I think, if emitted goog.require is correct, everything else will work fine. Thank you very much for the fix. I hope your GCC PR will be accepted soon. 1.10 looks very solid and fast.
Hide
David Nolen added a comment -

The Closure Compiler PR now has a test case. It might need some small tweaks after it gets reviewed but hopefully this fix can make it into the May release at the latest.

Show
David Nolen added a comment - The Closure Compiler PR now has a test case. It might need some small tweaks after it gets reviewed but hopefully this fix can make it into the May release at the latest.
Hide
Thomas Mattacchione added a comment - - edited

It seems that the PR to google closure has been reviewed https://github.com/google/closure-compiler/pull/2847

Show
Thomas Mattacchione added a comment - - edited It seems that the PR to google closure has been reviewed https://github.com/google/closure-compiler/pull/2847
Hide
Aleš Roubíček added a comment -

It seems that it works in latest cljs (1.10.339).

Show
Aleš Roubíček added a comment - It seems that it works in latest cljs (1.10.339).

People

Vote (2)
Watch (3)

Dates

  • Created:
    Updated: