Fork me on GitHub
#cljs-dev
<
2016-12-30
>
r0man19:12:34

I have a problem with the ClojureScript constants table (the constants_table.js file) being emitted after JavaScript that tries to use those constants. I get a lot of errors about calling functions on undefined. Those calls are all on Clojure keywords and symbols that get defined later in the same JavaScript file via the constants_table.js file. In the list of sorted dependencies of a module I would expect the constants_table.js file before those namespaces that are using keywords or symbols from the constant table. Does anyone know if this assumption is correct?

r0man19:12:37

I'm trying to update to the latest ClojureScript version and noticed this happening on advanced builds using Google Closure Modules since the Google Closure Compiler was updated CLJS-1762.

dnolen19:12:36

@bhauman so playing around with :foreign-libs expansion - there’s still a problem - figure out the provided name for the lib

bhauman19:12:15

@dnolen don't you also have to figure the dependencies as well?

bhauman19:12:47

@dnolen yeah but we should fall back to the JS conventions here correct?

dnolen19:12:05

@bhauman actually maybe I’m wrong here

dnolen19:12:21

between JS libs we don’t need to figure out dependencies

dnolen19:12:39

Google will figure that out based on imports etc. and will invent a provided name

bhauman19:12:48

it will!!?

dnolen19:12:49

the problem is what does the user do

bhauman19:12:07

my experiments doing that didn't work

dnolen19:12:16

@bhauman it should work just fine

dnolen19:12:56

but the imports needs to be relative I’m pretty sure, I tried this already with Node.js and it worked just fine a long while ago

bhauman19:12:57

Ok cool I get it the foreign dep doesn't have to be complete

bhauman19:12:16

yes relative to the root of the project

bhauman19:12:24

root of the env

dnolen19:12:57

lets set the JS stuff aside for now - I’m pretty sure works, or won’t be that hard - people are already doing this with ES6 code bases

dnolen19:12:14

the real problem I see here is how to require something

dnolen19:12:20

into ClojureScript

dnolen19:12:39

since you didn’t name any of the libraries in the directory

bhauman19:12:44

folks will expect to use the name of the file right?

dnolen19:12:02

and the libraries aren’t following a convention like node_modules

dnolen19:12:22

@bhauman if just name of file seems like you will clash

bhauman19:12:36

including the path

dnolen19:12:36

but maybe we shouldn’t be innovating here anyway

dnolen19:12:50

just the usual classpath stuff

dnolen19:12:59

@bhauman k trying this now

dnolen19:12:44

@r0man just sounds like a bug - though it also sounds very unlikely - since it would effect all production code

dnolen19:12:53

@r0man so come up with a way to repro and will look at it

r0man19:12:31

@dnolen yes, trying to find a minimal case. just not that easy 🙂 Trying to understand what's going on in the compiler at the moment. I think I found something suspicious, just not sure if my assumptions are correct.

r0man19:12:04

Would you aggree that at his point https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/closure.clj#L1202 the constants_table.js file should be before namespaces that make use of ClojureScript keywords or symbols? In my project that's not the case, which I think is strange.

dnolen19:12:15

@r0man oh right - sorry you’re talking about modules - yeah we probably need a special case here

dnolen19:12:29

constants table isn’t a real namespace and declares no dependency

dnolen19:12:51

if you’re not using modules then we have some hard-coded stuff to put it in the right place

dnolen19:12:29

so we probably need to replicate that

r0man19:12:41

yes, I mean modules. In my case this file is just somewhere in the middle of the sorted list. And namespaces that use keywords come before.

r0man19:12:15

ok, i'll digg into it. Good to know I'm on the right track

dnolen19:12:58

@r0man yeah I think your analysis is correct

dnolen19:12:16

the constant table is getting sorted into the wrong place on that line

dnolen20:12:51

perhaps the easiest thing would be to look at how hard it would be to just move the constants table into the right place

dnolen20:12:05

a generic thing isn’t all that important here since constants table is a real special case

r0man20:12:25

@dnolen Now I wonder where the constant table actually should be? I think the correct place would be for keyword constants directly after the definition of Keyword type and for symbol constants directly after the definition of the Symbol type.

dnolen20:12:47

constants table should always come right after cljs.core

dnolen20:12:56

cljs.core is completely static for a reason

dnolen20:12:39

not interested in doing things differently here like trying to splice stuff in

r0man20:12:33

So just placing it after cljs.core is enough?

dnolen20:12:47

yes, that’s how we do it outside of modules

r0man20:12:37

Ok, cool.

dnolen20:12:10

@r0man well it’s interesting actually

dnolen20:12:28

since there’s nothing here that makes it come before, other than being before

dnolen20:12:36

everyone else that depends on cljs.core

r0man20:12:39

Do you mean with "`cljs.core` is completely static for a reason" we only get away with this because symbols and keywords are not used at the top level?

r0man20:12:53

I would have thought we have a chicken and egg problem.

dnolen20:12:04

@r0man there’s nothing at the top level

dnolen20:12:16

any CLJS data structure value at the top level is a problem for dead code elimination

dnolen20:12:13

only JS values that Closure can definitely understand

dnolen20:12:50

anyways, I am somewhat surprised it doesn’t already work

dnolen20:12:58

but it’s because we do our own sorting of deps

dnolen20:12:04

but in the module case we don't

dnolen20:12:58

one thing to try before messing around with the code

dnolen20:12:37

would be to make a namespace for the constants table

dnolen20:12:26

it maybe that Google Closure just needs a provide + require to work in the constants table file

r0man20:12:33

@dnolen I tried that already

r0man20:12:55

I emitted a goog.provide at the top of constants table

dnolen20:12:04

you need a require too

dnolen20:12:13

provide is not enough of course

r0man20:12:35

but I think for google closure to pick it up correctly I would need to add a require as well in all namespaces that use it

dnolen20:12:55

it just depends on their sorting algorithm

r0man20:12:03

I can try that

dnolen20:12:31

the thing is it will be in the right order when we get to the module building

dnolen20:12:39

so module building will see constants table right after core

r0man20:12:50

just wasn't sure if this isn't a bit too much messing around with the javascript files

dnolen20:12:57

so crossing my fingers it just works when we have Closure dep sort

dnolen20:12:14

@r0man changing code emission is preferred to mucking around with the compiler in this case

dnolen20:12:25

the contents of the constants table file is not that important 🙂

r0man20:12:56

ok, I'll give it a try 🙂

r0man20:12:17

thanks for the help, was scratching my head all day long

r0man21:12:39

@dnolen I got it working by adding a goog.provide to the constants_table.js file and a goog.require to every ClojureScript file the compiler generates. I have 2 more questions

r0man21:12:26

1. Right now I use the namespace "constant-table" , same as the file. Should I use a name under cljs.core, like cljs.core.constants?

dnolen21:12:28

@r0man oh, I wasn’t suggesting add it as dep to every file - just whether the single require was enough

dnolen21:12:48

that is that the constants file requires cljs.core and it comes first in the module building anyway

dnolen21:12:20

to your other point yes cljs.core.constants would be fine

r0man21:12:58

@dnolen hmm, do you mean just a goog.provide("constants_table") and a goog.require("cljs.core") in the constants_table.js file?

r0man21:12:09

I tried that, but this isn't enough

r0man21:12:28

another namespace that uses keywords could also just require cljs.core and google closure could sort it the other namespace and constants_table in 2 different ways

r0man21:12:55

I think this is also the reason why this used to work in older versions of clojurescript.

dnolen21:12:23

this needs more thought, you should open a ticket and attach whatever you have

r0man21:12:25

I just got the problem with the closure compiler from 3 months ago

dnolen21:12:36

I don’t have time to think about this much more today

dnolen21:12:21

I’m happy to review the patch you have to see what I think

dnolen21:12:31

(not right now but I will if you attach it)

r0man21:12:58

ok, I'll add a patch in the next days.

r0man21:12:07

also done for today

thheller22:12:52

@dnolen with regards to constants_table, a while ago I wrote a Closure Compiler pass do optimize the constants for me without any help of the CLJS compiler

thheller22:12:09

fairly simple and has served me well

thheller22:12:17

not sure if you'd be interested

thheller22:12:57

nothing shadow-build specific in it, could be ported very easily

thheller22:12:07

but not self-host compatible so proably a no-go

thheller22:12:36

no need to worry about constants anymore though 😉

bhauman23:12:01

@dnolen: let me know what you find :)