:boom:

fixed here https://github.com/google/closure-compiler/pull/2620/files

cc @bendlas

@anmonteiro nice!

trying out a :modules build with the latest cljs. The good news is, it’s compiling :slightly_smiling_face:. But something seems amiss: it is creating a file, <project root>/target/cljsbuild-main.js, in addition to the modules I specified. which are appearing correctly in the output-dir, <project root>/resources/public/js/compiled. I’ve checked the guide and I don’t see any new instructions - is there a way to specify where this cljsbuild-main file goes?

:modules 
{:live-frame {:entries   #{maria.frames.live-frame}
               :output-to "resources/public/js/compiled/live.js"}
 :trusted-frame {:entries   #{maria.frames.trusted-frame}
               :output-to "resources/public/js/compiled/trusted.js"}}
:output-dir  "resources/public/js/compiled/out-modules-dev"

@mhuebert what is cljsbuild-main?

are you using lein-cljsbuild?

might just be a tooling issue

@anmonteiro that is an excellent question

I would try compiling with a naive build script

also, are you using a :main entry in your compiler opts?

because you shouldn’t

@juhoteperi this is actually a good question for boot-cljs too

it shouldn’t create its main namespace if :modules is specified

does it know how to handle that?

no :main entry

@mhuebert so you’re using lein-cljsbuild?

@anmonteiro yeah, i will try with a build script

https://github.com/emezeske/lein-cljsbuild/blob/4bbb6a117bbab4aa158ea15b325bb93d434e53dd/sample.project.clj#L91

@anmonteiro my bad, I was on 1.1.6 and this was an issue fixed in 1.1.7

awesome

glad it’s figured out!

:slightly_smiling_face:

is there anything we need to manually npm-wise, before compiling with the latest cljs? eg. npm install @cljs-oss

err @cljs-oss/module-deps

@mhuebert not if you specify :npm-deps and :install-deps true

nothing manual

ok

I’ve been seeing this on every compile: TypeError('Path must be a string. Received ' + inspect(path))

TypeError: Path must be a string. Received { './server.js': './server.browser.js' }

this processing of modules is getting so much better :slightly_smiling_face:

there are so many things that one can get stuck on

but the experience that’s on the way is going to be so smooth.

@mhuebert it’s more helpful to add a snippet containing the full stacktrace

also can you tell us what server is here?

@dnolen looks like that comes from react-dom

this is probably related to advanced browser field usage

@mhuebert what module is that

react-dom?

@anmonteiro yes

[email protected]

ok, more generally, I’m using my old (non-:modules) build now just to be consistent, and after upgrading to 1.9.908, my references to react have broken - eg. this expression, (.-prototype react/Component), generates Uncaught TypeError: Cannot read property 'Component' of undefined, where in that namespace I have (ns .. (:require .. react react-dom))

I can try to put together a minimal example but before that, is there anything that should have changed the way this is to be used?

@mhuebert nope, but remember you changed your React version

or are you saying you were using 16 before?

same react version

16

one second

also I have swapped the cljs versions between 1.9.908 / 854,

just changing that one line in project.clj & rebuilding

re: the earlier discussion about that warning with server.js and server.browser.js, I’ve been getting that in both versions (854 and 908) and it hasn’t seemed to cause any problem, it’s just always logged on compile

React 16 compiles in a reasonable amount of time as an npm dep (15.* was much slower)

I don’t have any issues w/ master when requiring lodash with an alias at the REPL

ok

I’ll try to pare this down to a small test case

@mhuebert trying this here

testing npm stuff is trivial w/ the Quick Start uberjar

@mhuebert what’s your :npm-deps?

so previously I was just separately running npm install [email protected] [email protected] and leaving it out of :npm-deps, but it should be the same with:

:npm-deps     {:react     "next"
               :react-dom "next"}

@mhuebert it works for me with 1.9.908 uberjar

the problem is react-dom

@dnolen, ok, I am also setting something up with the jar and will try to reproduce what I’m seeing

and looks related to the browser field

I can repro here

@mhuebert ah right I didn’t get so far as ReactDOM was just trying to narrow it down to actual issue

nothing to do :npm-deps in general

might be related to how we resolve things now

^ in module_deps.js

@anmonteiro are you reproducing the warning that I posted a snippet of, or the problem here: https://clojurians.slack.com/archives/C07UQ678E/p1502993363000129

this is the problem:

path.js:28
    throw new TypeError('Path must be a string. Received ' + inspect(path));
    ^

TypeError: Path must be a string. Received { './server.js': './server.browser.js' }

@mhuebert what can I reference in react-dom?

react-dom/div ?

@mhuebert not interested in anything else right now. Pretty sure when this is fixed everything else will work

@dnolen react-dom/render for example

ok yeah

react works for me react-dom fails

@anmonteiro I just return path.node.source, I don’t check that it’s a string, I wonder if that’s a probem?

checking

@dnolen I don’t know if the problem would be in Konan

trying this w/ master now

my repro was against master

I mean by making the change against master

@anmonteiro nope unrelated to konan logic

@dnolen I think I know what’s wrong

and my fault too :slightly_smiling_face:

let me confirm, 1 sec

if the problem is in @cljs-oss/module-deps then at least we don’t need another release :slightly_smiling_face:

it’s not unfortunately

I fixed the immediate bug but getting another one

@dnolen add aliasFields: ['browser'], to the resolver config

around line 64

in module_deps.js

yeah that works

and removing 'browser' from mainFields

so what’s reasoning here?

@dnolen I can put together a patch, there’s another wrinkle to be taken care of

why should browser be in aliasFields instead of mainFields?

https://github.com/webpack/enhanced-resolve/blob/4bc3d614329ca63c57bc1d5dcb737482df836370/test/browserField.js#L16-L19

^ that’s how they use it in the tests

ok, not much of an explanation though :slightly_smiling_face:

what’s the other wrinkle?

the wrinkle is we only wanna set this if target === 'nodejs'

I can paste a diff here

ah right, I was just looking at that

diff --git a/src/main/cljs/cljs/module_deps.js b/src/main/cljs/cljs/module_deps.js
index f7aa5fa0..dc5cf17b 100644
--- a/src/main/cljs/cljs/module_deps.js
+++ b/src/main/cljs/cljs/module_deps.js
@@ -8,8 +8,9 @@ let enhancedResolve = require('enhanced-resolve');

 let target = 'CLJS_TARGET';
 let filename = fs.realpathSync(path.resolve(__dirname, 'JS_FILE'));
-let mainFields =
-    target === 'nodejs' ? ['module', 'main'] : ['module', 'browser', 'main'];
+let mainFields = ['module', 'main'];
+let aliasFields =
+    target === 'nodejs' ? [] : ['browser'];

 // 
 let getDeps = function (src, {dynamicImport = true, parse = {sourceType: 'module', plugins: '*'}} = {}) {
@@ -61,6 +62,7 @@ let resolver = enhancedResolve.create({
   ),
   extensions: ['.js', '.json'],
   mainFields: mainFields,
+  aliasFields: aliasFields,
   moduleExtensions: ['.js', '.json'],
 });

this is what I have

but it doesn’t seem to fix everything

trying that

@dnolen sorry edited

copy again please

@anmonteiro only change is not inlining mainFields why does that matter?

because we use it below

in the for loop

got it

so this solves the immediate issue, but I think we still have a problem in index-node-modules-dir

I don’t have time to look at it right now but I can fix it until the end of the day

what’s the issue there?

basically we need to know that react-dom/server is provided by react-dom/server.browser.js

I’m getting this error now:

Exception in thread "main" java.lang.AssertionError: Assert failed: cljs.analyzer/foreign-dep? expected symbol got "react-dom/server"

but what changed that this is broken now?

we never supported this

React probably changed

ok but that seems to be different from what @mhuebert was saying

it works with 854 but not 908

I can confirm with this change react and react-dom requires works

right

not react-dom/server probably

@anmonteiro ah so you’re saying just react-dom/server 0.16 definitely didn’t work before

I think so

@anmonteiro 15.6.1 doesn’t work anymore

and it doesn’t have this browser thing

oops scratch that typo, checking

works for me?

can confirm

@dnolen we’re just not saying that "react-dom/server.browser.js" provides "react-dom/server"

@mhuebert ok 15.6.1. definitely works

we should do this in module_deps.js

I’m at work right now but I’ll fix later

@anmonteiro I pushed the other module_deps.js fix to master

cool

will take your patch when you have it - would be nice to have some tests for these cases

yeah I’ll definitely add tests

cool

@dnolen I’ll provide patches for this one and https://dev.clojure.org/jira/browse/CLJS-2326 which I found earlier today

I’ll open an issue so I don’t forget later

assigned both CLJS-2326 and CLJS-2327 to me

will fix later

k sounds good