Fork me on GitHub
#cljs-dev
<
2017-01-30
>
dnolen00:01:08

oh I know why

dnolen00:01:24

because of how the CommonJS stuff handles export

dnolen00:01:54

var foo = function() {}; modules.export = foo; pattern

dnolen00:01:00

will get compiled to

dnolen00:01:30

goog.provide(”module$foo”); module$foo$foo = function() {}; var module$foo = module$foo$foo;

dnolen00:01:17

put that in a closure and it won’t work

dnolen00:01:33

so maybe what we’re doing is wrong

dnolen00:01:40

and we should try to fix it in Google Closure

bhauman00:01:19

dang it's gonna start snowing soon here in NC

bhauman00:01:04

@dnolen btw about to release 0.5.9 with support for hot loading the foreign lib dirs

bhauman00:01:12

its pretty slick

richiardiandrea00:01:29

@bhauman that is a feature that we probably would like to have in boot-figreload as well I will peek at your code 😀

dnolen00:01:22

heh yeah Google Closure actually has a test for that code gen - I’ll probably ask around whether that’s necessary - it’s not clear to me what semantic it’s preserving by doing things that way

dnolen00:01:49

hrm actually I do understand why it was done this way

dnolen00:01:55

because in compiled mode goog.provide gets erased

dnolen00:01:49

so it seems the output of CommonJS preprocessed by Google Closure really is incompatible with Node.js here

anmonteiro00:01:53

just got Circle Color working with NPM deps

anmonteiro00:01:59

+ advanced compilation

anmonteiro00:01:33

surprisingly the problem is with optimizations :none 😄

anmonteiro00:01:47

I don't know how to work around the process.env.NODE_ENV stuff

anmonteiro00:01:02

@roman01la's trick worked for advanced

dnolen00:01:19

@anmonteiro just make a process.env ClojureScript namespace with (goog-define NODE_ENV “development”)

anmonteiro00:01:23

hrm, maybe I just need to set! process in window or something

anmonteiro00:01:39

I did that, but it doesn't get picked up with :none

anmonteiro00:01:43

I suppose it needs to be a global

anmonteiro00:01:01

or I'm just doing something wrong

anmonteiro00:01:11

might be dep order.. hrm

dnolen00:01:16

that’s right

dnolen00:01:29

you need to load that ns first - fortunately dep order is guaranteed now 🙂

anmonteiro00:01:39

:preload, maybe?

dnolen00:01:49

you could do that

dnolen00:01:01

or just have your main ns order the deps

anmonteiro00:01:21

right, I put it as the last require

anmonteiro00:01:43

let's see if I move it to first place

anmonteiro00:01:51

hrm, no dice

dnolen00:01:39

@bhauman I don’t have a good answer for the __filename, __dirname problem. If it turns out to be a huge issue I may just wire in special support for that under :target :nodejs

dnolen00:01:01

want to hear more feedback first and whether there aren’t other subtler problems with global require for everything first though

anmonteiro00:01:10

the Circle Color project is 33.8KB gzipped

dnolen00:01:30

huh wow really? even with the ClojureScript bits?

dnolen00:01:42

or maybe Circle Color isn’t really using anything

dnolen00:01:52

if it’s not, that makes sense

anmonteiro00:01:57

it doesn't use anything CLJS, I believe

dnolen00:01:04

ok then that’s expected

anmonteiro00:01:55

52KB gzipped if I add:

(enable-console-print!)

(println "hi")

anmonteiro00:01:01

which is in line with your results I believe

dnolen00:01:25

it’s not shabby that’s for sure 🙂

anmonteiro00:01:19

preloads seems to work for setting process.env

anmonteiro00:01:33

weird thing is I get this now: Uncaught TypeError: this._instantiateReactComponent is not a function

anmonteiro00:01:45

at ReactCompositeComponentWrapper$$module$$Users$anmonteiro$Documents$github$circle_color$node_modules$react_dom$lib$instantiateReactComponent.performInitialMount

anmonteiro00:01:31

a solution is also to exploit the :preamble option

anmonteiro00:01:35

to set process.env.NODE_ENV

anmonteiro00:01:44

but then again, only works for :simple or :advanced

mrg01:01:09

Hey guys. I'm trying out the new externs inference and I'm having trouble with this:

mrg01:01:37

How would I typehint this correctly?

mrg01:01:17

I tried this, but it didn't seem to work:

mrg02:01:13

I read that @bhauman - but I don't know how that would apply in this case?

bhauman02:01:43

I haven't actually used it myself.

bhauman02:01:00

but from that tutorial you definitely can't hint an expression

mrg02:01:27

ah I see what you mean

bhauman02:01:09

jquery is going to be rough in this regard

mrg02:01:48

May come in the future. I just wasn't sure if I was doing something wrong.

mrg02:01:51

Thanks, Bruce

thheller10:01:13

@mrg to be safe you should always stay away from js* (defn jquery [sel] (js/jQuery sel))

thheller10:01:38

that should make the whole inference more reliable .. but jquery is going to be tough regardless

dnolen13:01:35

@mrg externs inference is designed to work for your case

dnolen13:01:06

but the guide probably needs to clarify the singleton pattern

dnolen13:01:27

(def jQuery js/$) is all you need

dnolen13:01:47

you don’t need any other hints if you ever use jQuery

dnolen13:01:38

I guess currently it may warn about adding externs to Object but you can ignore these if you like

dnolen13:01:06

it probably wouldn’t be hard to suppress those warnings in the case of this pattern - a top level tainted value

thheller13:01:14

@dnolen chaining won't work without tags right? $('.thing').css().click().attr().etc()?

thheller13:01:29

translated to cljs -> obviously

dnolen13:01:52

@thheller it could with what I’m suggesting

dnolen13:01:59

treating singleton pattern as a special case

dnolen13:01:29

since it’s quite common

dnolen13:01:02

what I mean is - it already works 🙂

dnolen13:01:14

you’re just going to get warnings about Object externs

dnolen13:01:37

but we could suppress those for this pattern

thheller13:01:48

but you'll get externs for Object not jQuery?

dnolen13:01:11

but I don’t think that matters much

thheller13:01:40

well it matters in the sense that nothing named click will ever get renamed

thheller13:01:49

but that doesn't matter much yeah

mrg13:01:12

I'll give it a try. Thanks @dnolen

mrg14:01:13

@dnolen with (def jquery js/$) I am getting the warning WARNING: Cannot infer target type in expression (. (jquery ".slider") slider (clj->js {"full_width" false}))

dnolen14:01:54

@mrg hrm ok will have to think about that

anmonteiro19:01:19

@dnolen there's a problem in cljs.build.api/add-package-jsons where it won't find the package.json for packages which :main entry is not at the root of the package

anmonteiro19:01:55

i.e. if :main is lib/Foo.js, it'll try to look for package.json in lib and bail when not found

dnolen19:01:45

@anmonteiro yeah, I didn’t know how common that was - so I didn’t bother handling that case

anmonteiro19:01:08

seems common enough for these 4 packages 🙂

dnolen19:01:00

yes, but what will the patch do?

dnolen19:01:05

I didn’t see anything obvious here

anmonteiro19:01:09

still trying to figure that out

anmonteiro19:01:24

just checking one level up seems too naive

dnolen19:01:54

it’s hard to say what the best approach is

dnolen19:01:05

would have been better of module-deps just returned that for us

anmonteiro19:01:51

that's what I'm looking into now

anmonteiro19:01:58

any reason the module_deps.js file has to be a single line?

anmonteiro19:01:17

would clojure.string/replace fail otherwise?

dnolen19:01:17

@anmonteiro no reason other than I didn’t test what happens if it isn’t?

dnolen19:01:24

if it works fine that way, go for it

anmonteiro20:01:15

I think I have something that works

anmonteiro20:01:35

another benefit is also dropping the JSONStream dep 🙂

juhoteperi20:01:14

Foreign-deps from local deps.cljs files don't currently work with Boot-cljs because relative-name presumes that the files are under user.dir: https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/util.cljc#L152

anmonteiro20:01:32

hrm that's unfortunate

dnolen20:01:58

@juhoteperi where are they placed in Boot-cljs?

juhoteperi20:01:04

It would work if the function tried to resolve file from classpath directories if it is not under user.dir

juhoteperi20:01:27

in temp directories, e.g. resources/foo.js -> home/juho/.boot/cache/tmp/home/juho/Source/saapas/mv8/uanrg/foo.js

juhoteperi20:01:43

And home/juho/.boot/cache/tmp/home/juho/Source/saapas/mv8/uanrg/ is in classpath

dnolen20:01:04

but how can you compute anything relative here?

dnolen20:01:11

that’s the actual problem

dnolen20:01:21

it doesn’t matter where you put anything

dnolen20:01:31

just whether you can effectively prevent clashes

juhoteperi20:01:21

The foreign library contents are resolved from classpath, so file path relative to classpath directory it is inside should be unique enough

juhoteperi20:01:21

Or well, they are first resolved from classpath, and then from working directory (using reader)

dnolen20:01:56

@juhoteperi ok, how about a patch that uses classpath if the file isn’t a child of user.dir

juhoteperi21:01:30

Working on it

anmonteiro21:01:10

@dnolen OK if I delete add-package-jsons entirely?

anmonteiro21:01:25

it'll be in Git if we need to bring it back..

dnolen21:01:25

if it’s not needed anymore

anmonteiro21:01:45

putting a patch together now

anmonteiro21:01:48

I'll let you know

dnolen21:01:15

great thanks, will take a look later

anmonteiro22:01:50

figured out why consuming React from NPM doesn't work with :none

anmonteiro23:01:51

just moving the assignment to after the function declaration fixes it: https://github.com/facebook/react/pull/8895/files

Roman Liutikov23:01:33

@anmonteiro got here to ask about exact same issue, we gonna fix React! 😄

anmonteiro23:01:20

@roman01la first the PRs need to be accepted 🙂

anmonteiro23:01:46

I'm confident they will, the changes looks fairly simple

Roman Liutikov23:01:49

yeah, also they do want to bundle React using Closure, so it makes sense to fix all these hacky JS things

anmonteiro23:01:00

right. I saw that, and it's a good thing that actually helps our side 🙂