Clojurians
#cljs-dev
<
2017-08-17
>

This page is not created by, affiliated with, or supported by Slack Technologies, Inc.

dnolen04:08:35

@anmonteiro nice!

mhuebert17:08:33

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?

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’t

anmonteiro17:08:21

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

anmonteiro17:08:36

it shouldn’t 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’re 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’s figured out!

mhuebert17:08:11

:slightly_smiling_face:

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’ve 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 :slightly_smiling_face:

mhuebert17:08:28

there are so many things that one can get stuck on

mhuebert17:08:53

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

dnolen17:08:27

@mhuebert it’s 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

mhuebert17:08:22

@anmonteiro yes

mhuebert18:08:23

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))

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’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

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’t have any issues w/ master when requiring lodash with an alias at the REPL

mhuebert18:08:52

I’ll 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’s your :npm-deps?

mhuebert18:08:05

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

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’m 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’t 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’t check that it’s a string, I wonder if that’s a probem?

anmonteiro18:08:57

@dnolen I don’t 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’s wrong

anmonteiro18:08:48

and my fault too :slightly_smiling_face:

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’t need another release :slightly_smiling_face:

anmonteiro18:08:18

it’s 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’s reasoning here?

anmonteiro18:08:29

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

dnolen18:08:50

why should browser be in aliasFields instead of mainFields?

anmonteiro18:08:05

^ that’s how they use it in the tests

dnolen18:08:50

ok, not much of an explanation though :slightly_smiling_face:

dnolen18:08:53

what’s 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’t seem to fix everything

dnolen18:08:31

trying that

anmonteiro18:08:47

@dnolen sorry edited

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’t have time to look at it right now but I can fix it until the end of the day

dnolen18:08:08

what’s 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’m 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’re saying just react-dom/server 0.16 definitely didn’t work before

dnolen18:08:09

@anmonteiro 15.6.1 doesn’t work anymore

dnolen18:08:14

and it doesn’t 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’re 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’m at work right now but I’ll 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’ll definitely add tests

anmonteiro18:08:59

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

anmonteiro18:08:25

I’ll open an issue so I don’t 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