Fork me on GitHub
#cljs-dev
<
2017-03-02
>
richiardiandrea00:03:53

yeah let me know if you want me to try something else

anmonteiro00:03:07

actually the problem is not absolute files

anmonteiro00:03:25

it’s the usage of (System/getProperty “user.dir")

anmonteiro00:03:44

@richiardiandrea pushing a new patch real soon

anmonteiro00:03:53

btw thanks so much for giving the patch a go

richiardiandrea00:03:49

thanks for debugging it 🙂

anmonteiro00:03:01

I’m fairly confident on the state the patch is at now

anmonteiro00:03:15

works with cljs.jar, and Boot

anmonteiro00:03:28

able to consume several modules, no problem 🙂

richiardiandrea03:03:44

@anmonteiro et al. I created a repro for importing aws-sdk using CLJS-1960 (`mies` template) here: https://github.com/arichiardi/lumo-repros/pull/1

anmonteiro05:03:10

turns out package.json files can have a "browser" entry that specifies replacements for files if we're requiring that package for use in a browser

anmonteiro05:03:29

so in cljs/module_deps.js we'll have to use different resolvers according to ClojureScript's compilation target 🙂

anmonteiro05:03:59

this is gonna be a fun one to solve

Roman Liutikov11:03:10

@anmonteiro Did you test the patch with :optimizations :none or :simple? It seems like NPM packages couldn’t be resolved. I’m getting using undeclared var warnings.

Roman Liutikov11:03:50

@anmonteiro Works fine with advanced optimizations, except React gets an error, probably because of Closure Compiler

anmonteiro15:03:39

@roman01la I'll look into that today, can you paste the error or warnings you're getting?

anmonteiro15:03:33

@roman01la actually I didn't test with optimizations :advanced but I'm glad it works. :none or :simple works great here

Roman Liutikov15:03:28

(b/build "src"
              {:output-to "resources/public/js/compiled/fridge_client.js"
               :output-dir "resources/public/js/compiled/out"
               :npm-deps {:react “15.4.2”
                                   :react-dom "15.4.2"}
               :main 'app.core
               :optimizations :simple
               :pseudo-names true
               :closure-defines {'process.env/NODE_ENV "development"}
               :pretty-print false
               :verbose true})
(ns app.core
  (:require [process.env]
            [react :as React]
            [react-dom :as ReactDOM]))

(def App (.createElement React "h1" nil "HELLO!"))

(.render ReactDOM App
                  (.getElementById js/document “app”))

Roman Liutikov15:03:40

here’s build config and code

Roman Liutikov15:03:05

warnings

WARNING: Use of undeclared Var app.core/React at line 6 /Users/roman01la/projects/fridge/fridge-client/src/app/core.cljs
WARNING: Use of undeclared Var app.core/ReactDOM at line 8 /Users/roman01la/projects/fridge/fridge-client/src/app/core.cljs

anmonteiro15:03:38

@roman01la sure that's expected

anmonteiro15:03:51

use ReactDOM/render

anmonteiro15:03:58

ReactDOM is a namespace now, not an object

Roman Liutikov15:03:36

transition to NPM packages is going to be hard 😄

Roman Liutikov15:03:27

btw, are there going to be any problems with NPM modules and libraries which are using CLJSJS packages at the moment?

Roman Liutikov15:03:34

I mean wouldn’t it be a breaking change?

anmonteiro15:03:37

please clarify

anmonteiro15:03:05

do you mean consuming React from node_modules while using React from CLJSJS?

Roman Liutikov16:03:27

hmm, I mean libraries that will switch to NPM modules instead of CLJSJS will no longer work with older versions of ClojureScript, right?

anmonteiro16:03:57

That's correct

anmonteiro16:03:24

So it's not a CLJS breaking change

anmonteiro16:03:43

But libraries can introduce breaking changes if they want to

Roman Liutikov16:03:04

yeah, I just think this will cause very slow transition to NPM modules in ClojureScript libraries

Roman Liutikov16:03:18

not a big ideal in fact

anmonteiro16:03:52

Sure. But my patch doesn't even allow libraries to use NPM modules yet

anmonteiro16:03:18

I'll work on that support after this patch gets in

anmonteiro16:03:59

To clarify, we'll support a npm-deps option in upstream JARS via deps.cljs

dnolen16:03:41

people will need to be very careful about publishing stuff that depends on :npm-deps

dnolen16:03:51

I would argue :npm-deps is mostly for application developers

dnolen16:03:42

depending on very common shared things like React is also reasonable

dnolen16:03:59

but depending on lots of random stuff is going to be bad idea

dnolen16:03:07

since how we handle version conflicts will always be very primitive

anmonteiro16:03:41

@dnolen btw not sure when you're going to look at the node modules patch, but I have addressed the browser vs node resolver issues locally so the patch attached to CLJS-1960 is stale now

anmonteiro16:03:06

I'll let you know when I upload the updated one

anmonteiro16:03:31

Gotcha, still have some time to kick it around then :-)

spinningtopsofdoom18:03:52

@anmonteiro I think I found a issue with your patch and react-dom. react-dom has two entry points index.js (the main entry point) and server.js. If I try to use any of the react-dom server functionality I can't because it's not loaded. When I try to load it manually I get duplicate module errors from Google Closure

anmonteiro18:03:25

@spinningtopsofdoom how would you require it in JS?

anmonteiro18:03:48

require(’react-dom/server’);?

anmonteiro18:03:59

yeah that’s gonna need more hammock time 🙂

spinningtopsofdoom18:03:11

Like in the current js module guide

anmonteiro18:03:11

we’re not gonna be able to support that for now

anmonteiro18:03:37

but you can still fallback to what the guide does

anmonteiro18:03:12

the main problem with something like this is that you probably can’t require react-dom/server in a CLJS namespace

spinningtopsofdoom18:03:19

Yep but mixing the two methods will lead to duplication errors from Google Closure

anmonteiro18:03:38

and changing it to dots or dashes is too implicit for my taste

anmonteiro18:03:00

@spinningtopsofdoom so that’s definitely an issue that we can solve by making module entries a set

anmonteiro18:03:10

and I can address that in my patch

anmonteiro18:03:07

btw thanks for trying out the patch

anmonteiro18:03:14

lots of valuable input from everyone that tried it so far!

spinningtopsofdoom18:03:45

Cool the code I had to resolve it manually is

var ReactDOMServer = require("react-dom/server");

module.exports = {
  ReactDOMServer: ReactDOMServer
};
for the javascript file and
(def node-libs
  (let [entry {:file (.getAbsolutePath (io/file "src/libs/npm_deps.js"))
               :provides ["libs.npm-deps"]
               :module-type :commonjs}]
    (into [entry] (cc/node-inputs [entry]))))
to get the foreign module deps

anmonteiro18:03:42

@spinningtopsofdoom yep, but you still had a :npm-deps entry right?

anmonteiro18:03:57

and we merge those together creating duplicates

spinningtopsofdoom18:03:05

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

anmonteiro18:03:13

so it’s a simple fix