Fork me on GitHub
#cljs-dev
<
2017-08-29
>
juhoteperi11:08:08

@anmonteiro Have I understood the :npm-deps and :install-deps correctly: :npm-deps is used to define the deps for apps and libs (in deps.cljs), but alone doesn't do anything, :npm-deps are installed only when :install-deps is true, and the reason why :install-deps exists is that we don't want to install :npm-deps from deps.cljs without option in the project.

kommen11:08:42

is/was there consideration to make the path where :npm-deps looks for node_modules configureable?

juhoteperi12:08:04

@kommen I don't know about that, but: :npm-deps is only about installing Node packages, Node modules indexing is separate to that, and works even without :npm-deps

kommen12:08:03

yes, that is exactly my problem: from the same codebase we compile 2 apps: one for the browser and for nodejs. and some of our dependencies are not compatible with the Closure commonjs processing, so we can’t use node_modules for the browser target. but the for nodejs target we now would need to use :npm-deps. so the nodejs target needs node_modules there, where the browser target shouldn’t find them

juhoteperi12:08:18

Hmm yeah, that will be problematic

juhoteperi12:08:17

You can remove the node_modules before building the browser version as a workaround

kommen12:08:46

I did that

kommen12:08:07

but in development both run in parallel 🙈

dnolen12:08:46

@kommen I believe it should be possible to get the isolation you want if your browser build doesn’t actually require anything from node_modules

dnolen12:08:08

Also I guess in your case you actually need both builds during dev?

kommen12:08:33

yes, we need both builds during dev

kommen12:08:01

and yes, the browser build does require some of the same stuff as the nodejs build

dnolen12:08:25

@kommen but can’t you manage this by isolating your namespaces in your builds?

dnolen12:08:47

this way the nses bringing in non-browser node deps is only part of one specific build

juhoteperi12:08:38

Perhaps the browser nses require some of the same libs as used by node build? E.g. node uses react from node_modules and browser uses react from foreign-lib

dnolen12:08:16

@juhoteperi that doesn’t seem to agree with what @kommen said at 8:09am

dnolen12:08:26

so I’m still confused about the description of the problem

kommen12:08:31

we have one react library in the browser which Closure doesn’t like which relies on react comming from a foreign-lib. but for the nodejs reagent want to load react from node_modules

kommen12:08:54

and reagent is also used in the browser

dnolen12:08:32

@kommen oh, are you trying to abstract away React usage regardless of whether React comes from foreign-lib or node_modules?

juhoteperi12:08:16

Reagent (alpha) does that

kommen12:08:05

@dnolen yes, I think that is what it comes down to

kommen12:08:19

@juhoteperi yes, I’m on that alpha. but to have this working with nodejs target I need node_modules where ClojureScript tries to index them, right?

kommen12:08:31

which then, as far as I understand, breaks for the browser build, because ClojureScript sees react in node_modules and uses it, but then the not-closure-compatible react-component can’t work

dnolen12:08:14

@kommen I think that would need to be managed via the classpath?

dnolen12:08:26

i.e. browser build uses old reagent, node build uses reagent alpha

kommen12:08:43

possibly yes

dnolen12:08:45

@juhoteperi what was your plan for supporting that in Reagent? Maven classifier?

dnolen12:08:19

I can imagine this scenario being relatively common

kommen13:08:21

on another front, as a heads up, since 1.9.908 the Closure compiler crashes when turning on :infer-externs when compiling our project with optimizations. I filed an issue here: https://github.com/google/closure-compiler/issues/2629

juhoteperi13:08:36

@dnolen Reagent alpha uses foreign-libs, IF node_modules dir doesn't exist

juhoteperi13:08:07

No need for maven classifier or anything

dnolen13:08:10

@kommen did you try downgrading Closure Compiler to see if that resolves the issue?

dnolen13:08:47

@juhoteperi right but that seems suboptimal given @kommen report and future similar ones

dnolen13:08:11

this is why I suggested managing this via the classpath long ago rather than attempting any detection in lib

juhoteperi13:08:29

I think it would make sense to provide option to either disable node module indexing completely, or provide option to set Node Module indexing working directory

dnolen13:08:29

doing this manually in each lib is destined to create all kinds of problems

juhoteperi13:08:59

I'm open to other other solutions (which is why Reagent is still alpha) but I don't yet understand what the other solutions would be

dominicm13:08:32

@dnolen doing this via classpath makes it hard to have parallel node & browser builds in the same JVM, which is a drawback to that approach.

dnolen13:08:45

@kommen @juhoteperi is using multiple node_modules dirs standard?

kommen13:08:13

@dnolen I tried downgrading Closure, but I think compiling then failed due to api incompatibilities

dnolen13:08:29

@kommen sounds like this problem needs a lot more information before anybody is going to even start thinking about looking at it

dnolen13:08:44

I would file a corresponding issue in ClojureScript - but the same caveat applies - more information please

dnolen13:08:35

@dominicm yeah that’s a good point - forgetting same JVM doing two builds

juhoteperi13:08:11

I think there are cases where Node projects have multiple node_module dirs, or at least multiple package.json, e.g this mono repo using Lerna: https://github.com/material-components/material-components-web/tree/master/packages/mdc-base, each package has own package.json

juhoteperi13:08:30

I think a Node project could also have sub dirs for browser and node targets, and then use shared code from third folder

juhoteperi13:08:42

and browser and node targets would have their own package.json and node_modules

dnolen13:08:35

@juhoteperi I’m asking something different

dnolen13:08:42

different node_modules dirs at the same level

juhoteperi13:08:53

Yeah I don't know about that

juhoteperi13:08:02

Haven't seen it, and I'm not sure if npm provides configuration for that

dominicm13:08:06

Is there a different approach, where #?(:cljs/node (node_modules-blah) :cljs/browser (stuff)) is done?

dominicm13:08:19

@juhoteperi there's an env variable for it.

dnolen13:08:29

@dominicm can’t consider that, not supported by reader conditionals

dnolen13:08:35

@juhoteperi then option to disable node_modules processing seems the least boxing-in way to go

juhoteperi13:08:17

I think Closure has option to set root dirs for Node dependency resolution

dnolen13:08:05

@juhoteperi resolution is separate from :module-roots pretty sure and we already have an option for that

kommen13:08:45

@dnolen I know, tried to find out more information, but I’m not yet familiar in neither affected areas in both compilers. just wanted to track the issue somewhere in case other people run into the issue

juhoteperi13:08:27

Okay, I was wondering if Closure option defaults to . and is hardcoded to look under ./node_modules, or if it defaults to node_modules

juhoteperi13:08:37

It is the first, so it can't be changed to look at other folders

dnolen13:08:30

@kommen there’s might be an easy way to repro - just make a dummy JS project with the extern file we generate

dnolen13:08:42

doesn’t really require knowing anything about any of the compilers

dnolen13:08:49

just need something minimal

dnolen13:08:54

@juhoteperi hrm yeah that I’m not really aware of

kommen13:08:33

ah, I see what you mean. I only tried to create a repro case from scratch with no success

dnolen13:08:45

yeah that’ll be too hard 🙂

dnolen13:08:11

generated externs file gets placed in :output-dir pretty sure

dnolen13:08:12

@kommen well, I suspect there isn’t going to be an easy answer for your problem anytime soon - too many different things to consider

dnolen13:08:22

re: 2 builds issue

kommen13:08:20

alright, thanks. will need to figure something out then

dnolen13:08:33

I think you need to just give up on trying to abstract away React for the time being

dnolen13:08:49

just load it two different ways - you don’t need :npm-deps really for what you are doing anyway

dnolen13:08:29

Node.js stuff is a free-for-all, making :npm-deps work there was just sugary stuff

dominicm19:08:19

@juhoteperi could checking for the node modules directory also be problematic if I'm running a simultaneous unrelated build with node, and don't want reagent to use it at all? E.g. I'm doing postcss on one terminal. And boot dev in the other

juhoteperi19:08:55

@dominicm Depends on if you have node_modules dir present on the working dir

dominicm19:08:59

@juhoteperi yep, for postcss. This seems like a common enough scenario that could also happen. Would it be possible to switch on a compiler constant perhaps?

mfikes19:08:44

I wonder if js-keys should just delegate to goog.object/getKeys

mfikes19:08:12

(`goog`‘s appears to run significantly faster—sometimes 2×—, at least without :advanced involved)

mfikes20:08:19

Perhaps Rich was simply unaware of goog.object/getKeys when he wrote js-keys, basing it on goog.object/forEach and callbacks. (Knowing about getKeys you could see that he might have just skipped writing a dedicated js-keys.)

dnolen20:08:36

@mfikes probably, seems like a fine idea