Fork me on GitHub
#cljs-dev
<
2017-08-17
>
mhuebert17:08:33

trying out a :modules build with the latest cljs. The good news is, it鈥檚 compiling 馃檪. 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鈥檝e checked the guide and I don鈥檛 see any new instructions - is there a way to specify where this cljsbuild-main file goes?

mhuebert17:08:40

: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"

anmonteiro17:08:17

@mhuebert what is cljsbuild-main?

anmonteiro17:08:32

are you using lein-cljsbuild?

anmonteiro17:08:36

might just be a tooling issue

mhuebert17:08:46

@anmonteiro that is an excellent question

anmonteiro17:08:48

I would try compiling with a naive build script

anmonteiro17:08:06

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

anmonteiro17:08:10

because you shouldn鈥檛

anmonteiro17:08:21

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

anmonteiro17:08:36

it shouldn鈥檛 create its main namespace if :modules is specified

anmonteiro17:08:42

does it know how to handle that?

mhuebert17:08:55

no :main entry

anmonteiro17:08:23

@mhuebert so you鈥檙e using lein-cljsbuild?

mhuebert17:08:34

@anmonteiro yeah, i will try with a build script

mhuebert17:08:41

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

anmonteiro17:08:01

glad it鈥檚 figured out!

mhuebert17:08:37

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

mhuebert17:08:12

err @cljs-oss/module-deps

anmonteiro17:08:41

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

anmonteiro17:08:44

nothing manual

mhuebert17:08:58

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

mhuebert17:08:19

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

mhuebert17:08:42

this processing of modules is getting so much better 馃檪

mhuebert17:08:28

there are so many things that one can get stuck on

mhuebert17:08:53

but the experience that鈥檚 on the way is going to be so smooth.

dnolen17:08:27

@mhuebert it鈥檚 more helpful to add a snippet containing the full stacktrace

dnolen17:08:27

also can you tell us what server is here?

mhuebert17:08:17

@dnolen looks like that comes from react-dom

anmonteiro17:08:34

this is probably related to advanced browser field usage

anmonteiro17:08:04

@mhuebert what module is that

mhuebert18:08:23

ok, more generally, I鈥檓 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))

mhuebert18:08:54

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?

dnolen18:08:27

@mhuebert nope, but remember you changed your React version

dnolen18:08:42

or are you saying you were using 16 before?

mhuebert18:08:51

same react version

dnolen18:08:18

one second

mhuebert18:08:19

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

mhuebert18:08:31

just changing that one line in project.clj & rebuilding

mhuebert18:08:59

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

mhuebert18:08:21

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

dnolen18:08:50

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

mhuebert18:08:52

I鈥檒l try to pare this down to a small test case

dnolen18:08:02

@mhuebert trying this here

dnolen18:08:24

testing npm stuff is trivial w/ the Quick Start uberjar

dnolen18:08:10

@mhuebert what鈥檚 your :npm-deps?

mhuebert18:08:05

so previously I was just separately running npm install react@next react-dom@next and leaving it out of :npm-deps, but it should be the same with:

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

dnolen18:08:03

@mhuebert it works for me with 1.9.908 uberjar

anmonteiro18:08:41

the problem is react-dom

mhuebert18:08:45

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

anmonteiro18:08:47

and looks related to the browser field

anmonteiro18:08:52

I can repro here

dnolen18:08:17

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

dnolen18:08:23

nothing to do :npm-deps in general

anmonteiro18:08:33

might be related to how we resolve things now

anmonteiro18:08:48

^ in module_deps.js

mhuebert18:08:28

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

anmonteiro18:08:54

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' }

dnolen18:08:59

@mhuebert what can I reference in react-dom?

dnolen18:08:23

react-dom/div ?

anmonteiro18:08:33

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

anmonteiro18:08:43

@dnolen react-dom/render for example

dnolen18:08:30

react works for me react-dom fails

dnolen18:08:22

@anmonteiro I just return path.node.source, I don鈥檛 check that it鈥檚 a string, I wonder if that鈥檚 a probem?

anmonteiro18:08:57

@dnolen I don鈥檛 know if the problem would be in Konan

dnolen18:08:29

trying this w/ master now

anmonteiro18:08:45

my repro was against master

dnolen18:08:55

I mean by making the change against master

dnolen18:08:31

@anmonteiro nope unrelated to konan logic

anmonteiro18:08:44

@dnolen I think I know what鈥檚 wrong

anmonteiro18:08:48

and my fault too 馃檪

anmonteiro18:08:54

let me confirm, 1 sec

dnolen18:08:55

if the problem is in @cljs-oss/module-deps then at least we don鈥檛 need another release 馃檪

anmonteiro18:08:18

it鈥檚 not unfortunately

anmonteiro18:08:27

I fixed the immediate bug but getting another one

anmonteiro18:08:39

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

anmonteiro18:08:48

around line 64

anmonteiro18:08:51

in module_deps.js

anmonteiro18:08:08

yeah that works

anmonteiro18:08:12

and removing 'browser' from mainFields

dnolen18:08:26

so what鈥檚 reasoning here?

anmonteiro18:08:29

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

dnolen18:08:50

why should browser be in aliasFields instead of mainFields?

anmonteiro18:08:05

^ that鈥檚 how they use it in the tests

dnolen18:08:50

ok, not much of an explanation though 馃檪

dnolen18:08:53

what鈥檚 the other wrinkle?

anmonteiro18:08:20

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

anmonteiro18:08:31

I can paste a diff here

dnolen18:08:49

ah right, I was just looking at that

anmonteiro18:08:54

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'],
 });

anmonteiro18:08:57

this is what I have

anmonteiro18:08:03

but it doesn鈥檛 seem to fix everything

dnolen18:08:31

trying that

anmonteiro18:08:53

copy again please

dnolen18:08:34

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

anmonteiro18:08:42

because we use it below

anmonteiro18:08:48

in the for loop

anmonteiro18:08:37

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

anmonteiro18:08:00

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

dnolen18:08:08

what鈥檚 the issue there?

anmonteiro18:08:16

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

anmonteiro18:08:26

I鈥檓 getting this error now:

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

dnolen18:08:34

but what changed that this is broken now?

anmonteiro18:08:46

we never supported this

anmonteiro18:08:53

React probably changed

dnolen18:08:16

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

dnolen18:08:42

it works with 854 but not 908

dnolen18:08:38

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

anmonteiro18:08:52

not react-dom/server probably

dnolen18:08:07

@anmonteiro ah so you鈥檙e saying just react-dom/server 0.16 definitely didn鈥檛 work before

dnolen18:08:09

@anmonteiro 15.6.1 doesn鈥檛 work anymore

dnolen18:08:14

and it doesn鈥檛 have this browser thing

dnolen18:08:15

oops scratch that typo, checking

anmonteiro18:08:18

works for me?

dnolen18:08:08

can confirm

anmonteiro18:08:10

@dnolen we鈥檙e just not saying that "react-dom/server.browser.js" provides "react-dom/server"

dnolen18:08:17

@mhuebert ok 15.6.1. definitely works

anmonteiro18:08:20

we should do this in module_deps.js

anmonteiro18:08:38

I鈥檓 at work right now but I鈥檒l fix later

dnolen18:08:58

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

dnolen18:08:15

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

anmonteiro18:08:28

yeah I鈥檒l definitely add tests

anmonteiro18:08:59

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

anmonteiro18:08:25

I鈥檒l open an issue so I don鈥檛 forget later

anmonteiro19:08:14

assigned both CLJS-2326 and CLJS-2327 to me

anmonteiro19:08:17

will fix later

dnolen19:08:41

k sounds good