Fork me on GitHub
#cljs-dev
<
2017-07-04
>
potetm01:07:35

@dnolen I'm not sure what the proper protocol is, but I wanted to follow up and let you know that I submitted that bug+patch: https://dev.clojure.org/jira/browse/CLJS-2162

potetm01:07:10

Please let me know if I left anything out.

potetm01:07:47

Also, there are a number of issues outstanding on the browser repl. I realize that's not exactly a hotspot of activity with figwheel out there, but I probably have enough knowledge to tackle any of them. I'm curious if you'd be interested in having some particular ones resolved, or if it would just be a distraction.

anmonteiro04:07:14

@dnolen wanted to get your thoughts on the new goog.module stuff where goog.require returns a value now

anmonteiro04:07:17

what’s the value proposition there? is it maybe at odds with Clojure(Script) namespaces?

Roman Liutikov07:07:08

@dnolen I was experimenting with modules and had a case where loader/load is called before loader/set-loaded!. Not sure why, but a callback to load is never called. Even though requested module was loaded with all its dependencies.

Roman Liutikov07:07:06

@dnolen Doing so seems wrong to me anyway, but I believe this should be clarified somewhere to avoid more confusion. Maybe compiler warning would help here

Roman Liutikov07:07:34

@dnolen In my case this was not obvious because I have a call to load in Reagent component which is being rendered right after component declaration. Later I figured out that that load was called before set-loaded! because the first render is always synchronous in React.

dnolen12:07:51

@roman01la you need to make something minimal that demonstrates the problem, as it is I don’t really understand what you are saying

dnolen12:07:52

I’m confused because you haven’t even mentioned what modules were involved (A, B or whatever)

Roman Liutikov12:07:35

ok, I’ll drop a link later today

dnolen12:07:10

@anmonteiro I guess that’s attractive for some reason for Closure devs? I’ve seen it before and I’ve more or less ignored it since I don’t see them changing the standard lib to that format anytime soon. Unless of course you’ve heard otherwise?

dnolen12:07:51

@potetm first glance patch looks ok, thanks! If you want tackle some outstanding issues with browser REPL / server go for it!

dnolen13:07:38

@petterik well looks like no need for YourKit, just needed a memoize, instead of 400s it now takes 400ms

dnolen13:07:03

try master when you get a chance

petterik14:07:48

@dnolen Nice! Perf issue is fixed: Successfully compiled "/Users/petterik/Github/......./main.js" in 145.941 seconds.

dnolen14:07:10

now to actually get it working right? 🙂

petterik14:07:23

Not getting React is not being added to cljs_base.js for me. Have you seen this problem?

petterik14:07:07

cljsjs.react is added to :cljs_base module though

dnolen14:07:18

that is React

petterik14:07:00

Yes, but the minified react in cljsjs.react isn't prepended to cljs_base.js

dnolen14:07:22

yeah maybe I broke that looking

dnolen14:07:24

oh yeah, duh - ok fixing

dnolen14:07:49

@petterik thanks again for your modules/sources from yesterday, looks like a very non-trivial project 😉

dnolen14:07:38

@petterik try master, it was just a dumb bug

dnolen14:07:46

was blowing :foreign-deps from :cljs-base

petterik14:07:32

You're welcome! Couldn't have built this project without cljs and om.next 🙂 Launching this month!

petterik14:07:34

Trying master now

dnolen14:07:55

awesome 🙂

petterik15:07:03

@dnolen Fixed! React is now in cljs_base.js

petterik15:07:36

Now it seems like I've got some application code that needs changing

juhoteperi15:07:18

@dnolen Have you started with js-transforms load code already? Or should I take a look at implementing that?

dnolen15:07:36

@juhoteperi if you want to do that be my guest 🙂 was just thinking about that

juhoteperi15:07:08

I'll have a look at it. Should be quite simple.

juhoteperi15:07:47

Did you have idea about validating the keys: "We should only support namespaces transform keys". As the files are just eval'd and just contain defmethod there is nothing to validate during loading these files?

dnolen15:07:35

@juhoteperi yeah … it seems ok

dnolen15:07:29

@juhoteperi given that you need to get at a JS engine and who knows what I else I don’t think trying to be declarative is productive

juhoteperi15:07:45

@dnolen That is what I thought also

dnolen15:07:01

@juhoteperi OK! yeah, don’t worry about namespaced keys - if people mess it up it will be obvious

dnolen15:07:34

(well maybe not obvious when things go wrong - but we can provide guidance here)

juhoteperi15:07:59

Default method already provides a warning about missing preprocess handler

dnolen15:07:05

@roman01la what am I supposed to do, what am I looking at, how do I reproduce, etc. etc.

dnolen15:07:11

I have zero context here - just some files

dnolen15:07:55

just glancing over this it look exactly like the guide that I wrote up

Roman Liutikov15:07:08

Sure. So if compile the project and open it in the browser you’ll probably see a message in the console, once a module is loaded via loader/load.

dnolen15:07:08

except that you have something between :cljs_base and your stuff, :common

Roman Liutikov15:07:08

The problem is that loader/load is called before injected loader/set-loaded!, so you’ll see that modules are fetched twice (via script tag and xhr)

dnolen15:07:49

I do not understand what you are saying

dnolen15:07:55

you need to much more clear

dnolen15:07:06

load / set-load without talking about what is meaningless

dnolen15:07:12

abstractly what are you saying

dnolen15:07:27

(load/load :a) (loader/set-loaded! :a) ???

dnolen15:07:31

this obviously meaningless

dnolen15:07:12

so what is the exact scenario here?

Roman Liutikov15:07:39

ok, so the module :app has this code (loader/load :page callback), that is called before (loader/set-loaded! :app), which is injected by compiler

Roman Liutikov15:07:19

both :app and :page depend on :common which depends on :cljs_base

Roman Liutikov15:07:40

because :app calls (loader/load :page callback) before (loader/set-loaded! :app) it causes ModuleManager to load not only :page but also :common and :cljs_base

dnolen15:07:47

but let’s rewind

Roman Liutikov15:07:48

so the first thing is that modules are fetched twice: first via script tag and then by ModuleManager

dnolen15:07:03

this example that you’ve given doesn’t make any sense to me

dnolen15:07:20

this zip I mean

dnolen15:07:35

calling load immediately like this seems gratuitous

dnolen15:07:46

what I want to understand is how did this happen your application?

dnolen15:07:57

because you had an immediate load side effect at the top level?

dnolen15:07:18

if so why would you do this? you might as well have an explicit dependency in that case

juhoteperi15:07:18

(defn load-js-transforms! []
  (doseq [url (get-js-transforms*)]
    (with-open [rdr (io/reader url)]
      (binding [*ns* (create-ns (symbol (random-string 5)))]
        (load-string "(clojure.core/refer 'clojure.core)")
        (load-reader rdr)))))
Okay, getting the list of files is easy. But what is the best way to load them? They can't use ns as they won't be in correct path relative to classpath, but when loading the code, a namespace should be used so cljs.closure is not contaminated with requires/vars from loaded files. This code works, but using random-string for namespace names is bad idea as it will create new namespace each time this function is called.

dnolen15:07:54

@juhoteperi should just copy Clojure’s loading of data literal files

juhoteperi15:07:17

Hmm, right didn't check there, just looked at Cljs data literal file reading

Roman Liutikov15:07:23

@dnolen In the app I had a Router component which loads modules depending on which route is active now. When the app loads initially the very first render in synchronous. Because (loader/set-loaded! :app) is injected into the end of the file and component is rendered synchronously, module load is fired before (loader/set-loaded! :app). I think this is a common scenario to load components in route-based code splitting

dnolen15:07:38

@roman01la we’re going to need better reason than random routing stuff

mfikes15:07:39

Draft of copy introducing the new aget, aset warnings for consideration for the official blog. It may be short enough for embedding in a larger post, and it is also long enough to potentially stand on its own: https://gist.github.com/mfikes/ba5e999eeef8295ee9d8d53af94ebc18

dnolen15:07:29

@roman01la all the Closure docs for ModuleManager show setLoaded coming at the end of namespace far as I know, top level loads before hitting the end of the file seems like a no-no

dnolen15:07:51

@roman01la you can also call set-loaded! yourself

juhoteperi15:07:14

load-data-readers doesn't help with this as data_readers files will only read a single object form the file and that must be a map.

dnolen15:07:53

@juhoteperi ah right because it assumes you already loaded the required fns explicitly

dnolen16:07:41

@juhoteperi clojure.core/load ?

Roman Liutikov16:07:29

@dnolen Sure I did exactly this. That's why I think a warning could be useful or at least there should be more info on that. ModuleManager is hidden behind cljs wrapper and it's not obvious how it behaves in such cases.

juhoteperi16:07:49

The example I pasted already uses load-reader which is easier to use with URLs from getResources and this works

juhoteperi16:07:07

But load-reader (and I think load) will evaluate the code in current namespace?

Roman Liutikov16:07:10

Should be very confusing for someone coming from Webpack for example.

dnolen16:07:30

@roman01la but a warning for what, and how would we detect such a thing?

dnolen16:07:40

this just seems like something to document to me

Roman Liutikov16:07:07

@dnolen Could be a runtime warning in development. ModuleManager keeps track of loaded dependencies, we can notify about load call if current module is not loaded yet

dnolen16:07:17

@juhoteperi ah right - hrm I dunno might need just play around wit this

dnolen16:07:32

@roman01la there is no such thing as “current module”

dnolen16:07:09

modules are not reified in anyway - it’s not possible to determine

dnolen16:07:25

at least not without a whole bunch of injection stuff that I’m not interested in at all

dnolen16:07:58

@roman01la … we could make load a macro I guess

dnolen16:07:28

and people would have to drop down to the ModuleManager API for more dynamic stuff

petterik16:07:18

@dnolen It's working! Last problem was just sloppiness integrating the cljs.loader ns.

petterik16:07:39

I'm also using the cljs.loader/loaded? and prefetch functions

petterik16:07:09

I'm using cljs.loader/loaded? in my Router's shouldComponentUpdate. Not rendering if it's not loaded yet

petterik16:07:51

My first render happens after the callback to cljs.loader/load with my initial route

dnolen16:07:58

@petterik or right looking at the patch

rauh16:07:21

@roman01la Wouldn't your problem be solved by just dropping the load call into a microtask?

dnolen16:07:33

@petterik have you submitted your CA?

dnolen16:07:48

@roman01la we could make load validate if it’s called statically (i.e. macro + runtime fn)

dnolen16:07:05

so we could validate the common case

petterik16:07:16

@dnolen I signed this one yesterday before creating the patch Clojure CA between Rich Hickey and Petter Eriksson is Signed and Filed!

Roman Liutikov16:07:28

@rauh it would, there are multiple solutions if you know where is the problem :)

juhoteperi16:07:38

@dnolen Here is one solution using classpath relative path of file munged as namespace name: https://github.com/Deraen/clojurescript/commit/bb94e566a8827d0356740491e2506fba02faa109

juhoteperi16:07:40

Oh except all the js_transform files from jars will have same classpath relative path...

dnolen16:07:15

@juhoteperi something like that seems like the right direction

juhoteperi16:07:24

It will probably work without relative-name, the namespace names might be very long but I hope that won't cause problems

Roman Liutikov16:07:14

@dnolen not sure how that would look like, but I'm up to any improvement :)

dnolen16:07:45

@roman01la nothing would change

dnolen16:07:49

but you would get your error 🙂

dnolen16:07:57

@juhoteperi what about js_transform_GENSYM?

juhoteperi16:07:20

@dnolen The name should be stable between load-js-transforms! calls so that it won't create new namespaces each time build is called. But could work if the names were generated in the memoized fn.

juhoteperi16:07:34

Or there could be a atom for URL -> symbol mapping

Roman Liutikov16:07:37

@dnolen Hehe. This might doesn't seem like a big deal, but devs would appreciate this, especially when it's a new thing in cljs, imo

juhoteperi16:07:12

Or the URL -> symbol mapping could be stored in compiler state

dnolen16:07:47

@roman01la will think about it

dnolen16:07:15

@juhoteperi why not just SHA the URL?

dnolen16:07:34

if we can get stable name w/o mucking around with other stuff - better

juhoteperi16:07:43

Ha, that would also work. The result will be shorter than mungle-path.

juhoteperi16:07:10

loading file:/home/juho/Source/clojurescript/src/test/clojure/js_transform.clj          js_transform_7596B998B31634FDE39058D5B7FA3055C958148C                                                                                                                                                      
loading jar:file:/home/juho/.m2/repository/cljsjs/babel-standalone/6.18.1-3-SNAPSHOT/babel-standalone-6.18.1-3-SNAPSHOT.jar!/js_transform.clj        js_transform_9CA4F1F139C72C4EC1A9BA1AF2DB184C7FA20DFE 
Seems okay

thheller16:07:47

for the record I think js_transform.clj is a bad idea. IMHO we should just allow :preprocess some.qualified/symbol (maybe even as a vector). the compiler can then load the namespace via require and find-var expecting to get an fn.

juhoteperi16:07:43

@thheller Yes, I think that might be cleaner solution

juhoteperi16:07:28

With this approach, the file is loaded (evaluated) each time cljs.closure/build is called, require would automatically only eval the first time

dnolen16:07:33

I’m fine with that too

juhoteperi16:07:52

I'll create another branch with that version

juhoteperi16:07:18

Another benefit is that if the loading is done when preprocess value is found, it will be lazy and only those namespaces that are really needed are loaded.

thheller16:07:27

I’m have a experimental branch that just builds on top of babel + .babelrc

thheller16:07:44

since that seems to be common and JS tools already do that

juhoteperi16:07:57

Initializing JS env in Babel-standalone file probably takes some time so running that everytime cljsjs/babel-standalone is present in classpath might be bad idea

juhoteperi16:07:33

(Except cljsjs code uses delay so it will be only created when calling js-transform first time)

dnolen16:07:01

@juhoteperi if we’re going to support :preprocess as symbol then we don’t need to be tied to js-transform multimethod

dnolen16:07:16

and we can also think about threading a new argument for sharing information between transforms

dominicm16:07:47

I noticed this morning that :file is an absolute path in cljs, but is classpath relative in clojure. I couldn't find any reference to this being different anywhere. Is this a bug? I did notice a blog post from 2xx that hinted that :file was relative, but it wasn't by 3xx.

dnolen16:07:24

@dominicm I don’t know why it matters

dnolen16:07:47

but we need absolute path when interacting with Closure in some cases

dominicm16:07:16

@dnolen for tooling. For example in boot the file is in a temporary path, but you actually want to open the real path.

juhoteperi16:07:29

@dominicm This is tooling issue

dominicm16:07:23

@juhoteperi if absolute paths are necessary then yep. I wasn't sure they were. I can move this to #boot I guess now. But is there anything that can be done to control the resulting metadata. I've made a PR to cider which attempts to convert an absolute path to a classpath uri, but it's not great. Come to think of it, it probably belongs in cljs-tooling

dnolen16:07:47

@juhoteperi before just hacking this out we might want to just write up what this will look like for end users

dnolen16:07:23

maybe we don’t need extra arg - but it does seem useful to share engine/babel in some cases?

juhoteperi16:07:29

Another useful feature is for the implementations to be able to read options from the ijs map, like the babel example:

:foreign-libs [{:file "src"
                :module-type :es6
                :preprocess :cljsjs.babel-standalone/babel
                :cljsjs.babel-standalone/babel-opts {:presets ["react" "es2016"]}}]

juhoteperi16:07:44

Which is already possible

dominicm16:07:23

@juhoteperi if absolute paths are necessary then yep. I wasn't sure they were. I can move this to #boot I guess now. But is there anything that can be done to control the resulting metadata. I've made a PR to cider which attempts to convert an absolute path to a classpath uri, but it's not great. Come to think of it, it probably belongs in cljs-tooling

mfikes16:07:43

Perhaps the docstring I added to unsafe-get said too much (given that it could be used on arrays)

mfikes16:07:09

"Efficient alternative to goog.object/get which lacks opt_val and emits
  unchecked property access.

dnolen17:07:10

“INTERNAL. Compiles to JS property access using bracket notation. Does not distinguish between object and array types and not subject to compiler static analysis.”

dnolen17:07:21

@mfikes ^ I have this docstring locally - what do you think?

anmonteiro17:07:14

@juhoteperi FWIW, if you're still thinking about using the SHA we probably just need the first 7 chars like we do here: https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/analyzer.cljc#L3501

anmonteiro17:07:30

@dnolen I don't have a compelling reason to use goog.module, just something I was reading about yesterday. A big advantage to me is that it would probably enable CLJS to output commonJS if we ever wabted

anmonteiro17:07:45

Since the formats don't look very different to me

anmonteiro17:07:12

Provided the core Closure lib moves to it I suppose

anmonteiro17:07:11

Btw FWIW we've been using :fn-invoke-direct since 1.9.671 without any problems /cc @rauh

richiardiandrea18:07:26

We like this feature a lot so so much.

dnolen18:07:31

@anmonteiro generating CommonJS is interesting, but to me the problem is still that who would want to consume it? - we generate a lot of code and most JS bundling tools just aren’t designed to handle it well

anmonteiro18:07:24

I can see a future where generating CommonJS modules just integrates better with the entire ecosystem

anmonteiro18:07:49

e.g. embedding a CLJS app in a JS one

anmonteiro18:07:29

I’m sure e.g. Webpack could handle our code if we generated CommonJS

dnolen18:07:02

@anmonteiro yeah that’s an interesting reason - just need more investigation about what the repercussions would be

juhoteperi18:07:19

@anmonteiro Wouldn't generating ES6 modules instead of CJS be more future proof? Would work in browsers directly,.

anmonteiro18:07:24

I don’t even know if it’s feasible today

anmonteiro18:07:38

just that the goog.module thing really resembles commonJS

anmonteiro18:07:58

maybe a summer of code project for 2018 🙂

anmonteiro18:07:28

yeah ES6 would be even better

anmonteiro18:07:46

and really suits Closure in that dependencies need to be known ahead of time

thheller19:07:15

shadow-cljs can turn CLJS code into something that commonjs tools can consume

thheller19:07:21

eg. webpack or node directly

thheller19:07:42

create-react-app, create-react-native-app all work out of the box without config

thheller19:07:03

it just prepends a few lines of ugly code 😉

thheller19:07:09

but it works

dnolen19:07:18

yeah for the non-web cases I can definitely see the value

anmonteiro19:07:27

even for the web there’s value

thheller19:07:41

it even works with :advanced

anmonteiro19:07:48

if your goal is to use Webpack to package a bundle

dnolen19:07:56

Value maybe, but do people actually care?

anmonteiro19:07:05

you can just emit commonJS modules and have Webpack take care of the rest

dnolen19:07:09

build stuff is like a one time pain for any real project

anmonteiro19:07:15

granted, it’s introducing one extra tool…

dnolen19:07:47

maybe people are clamoring for better Webpack integration but to be honest I’m not hearing it that much

anmonteiro19:07:51

@thheller TBH I’ve been meaning to look at shadow but still haven’t had a chance

anmonteiro19:07:12

what’s the fastest way I can get acquainted with it? 🙂

thheller19:07:13

my argument was that it should be easier to use CLJS in a project that already has lots of JS with tools for it

dnolen19:07:26

I mean argument sounds rationale yes

anmonteiro19:07:32

^ but this is only a fraction of the people

dnolen19:07:38

but I’m just talking about the market of typical Clojure users

anmonteiro19:07:46

we shouldn’t necessarily optimize for the 1% IMHO

dnolen19:07:50

do Clojure users care about Webpack … I seriously doubt it

Jon02:07:47

dnolen: please take us into consideration, I'm self-taught to work on front-end, nearly zero experience in JVM. I know several people similar to me in China learning ClojureScript without really using Clojure. Meanwhile Webpack is already our everyday life.

Jon02:07:40

I'm fine with all my code in cljs when toolchains covered all my work. However sometimes I ran into situations that are not covered, first time I think is Webpack has done that, if I can get CommonJS result, I can do that with Webpack.

dnolen19:07:15

my impression consistently is that people are happy to have CLJS do all this work

dnolen19:07:21

and zero interest in JS tooling

anmonteiro19:07:23

my commonJS / ES6 module suggestion was more of a long term goal / north star to have in mind

thheller19:07:26

@anmonteiro probably by checking out one of the shadow-cljs-examples and running it? 🙂

thheller19:07:01

made a comparison to the code-splitting sutff

anmonteiro19:07:05

maybe we can move this somewhere else, but I have trouble with declarative stuff. is there a programmable API?

dnolen19:07:20

all this said I agree there’s no reason to be close minded about it

dnolen19:07:36

and if there’s small population of users that would be benefit - I’m not against it

thheller19:07:09

@anmonteiro the docs for the low-level stuff are non-existent but this namespace is sort of the official API https://github.com/thheller/shadow-cljs/blob/master/src/main/shadow/cljs/devtools/api.clj

thheller19:07:24

can use it in a CLJ REPL

anmonteiro19:07:38

gotcha, I’ll have a look later

anmonteiro19:07:48

thanks for indulging 🙂

anmonteiro19:07:00

@dnolen just got this working locally:

(ns foo.core
  (:require ["react" :refer [createElement]]
            ["react-dom/server" :refer [renderToString]]))

(enable-console-print!)

(println (renderToString (createElement "div" nil "Hello World!")))

anmonteiro19:07:41

what’s better for you to review? attached patch to JIRA or GitHub diff?

dnolen20:07:55

@anmonteiro whoa, JIRA patch is fine

anmonteiro20:07:14

let me clean it up

juhoteperi20:07:22

@dnolen @thheller Did you check my notes about js-transforms? Opinions on multimethod + keywords vs symbol + function? https://gist.github.com/Deraen/9cf7d04221450bc10ac571399a483c4d

dnolen20:07:54

@juhoteperi I have to run will be take look later today or tomorrow

dnolen20:07:01

thanks for writing that up

juhoteperi20:07:28

Great. I myself prefer symbols+functions a bit over keywords+multimethod. Should be a more obvious to users that when they see the symbol in foreign-libs map, it refers to a function.

anmonteiro20:07:47

example included in the samples/ folder

anmonteiro21:07:01

surprisingly small patch too

juhoteperi21:07:39

@anmonteiro Did you try case where CJS module only exports single function? Like create-react-class

anmonteiro21:07:54

in that case you need to alias

anmonteiro21:07:29

or rather, if the module is of the form "react-dom/server", you need to alias

anmonteiro21:07:42

let me try that real quick and amend the patch if it doesn’t work

juhoteperi21:07:04

Works currently so probably it will work with the patch

juhoteperi21:07:20

This patch only support JS Modules with this string require?

anmonteiro21:07:35

what do you mean?

anmonteiro21:07:45

It preserves backwards compatibility if that’s what you’re asking

juhoteperi21:07:58

Doesn't work with non-js-module foreign libs

anmonteiro21:07:00

you can still (:require [react])

anmonteiro21:07:27

I think it might

anmonteiro21:07:30

but I didn’t try

anmonteiro21:07:50

actually it might not work, but could be changed to work

anmonteiro21:07:18

but my reasoning is: if you have a foreign lib with an explicit :provides, you can just require it with symbol

juhoteperi21:07:20

Would be very valuable if libraries can use same require forms for CommonJS and browser-ready-JS-files

juhoteperi21:07:02

e.g. reagent would use (:require ["react"]) and this would work both with cljsjs/react and :npm-deps {"react" ...}

juhoteperi21:07:09

(if this was implemented, cljsjs/react would provide the same "namespaces" as commonjs modules)

juhoteperi21:07:56

Problem is that there is no clean mapping between the name (`react`, react-dom, react-dom/server) and the object the foreign lib file provides (`React`, ReactDOM, ReactDOMServer).

thheller21:07:44

@juhoteperi wrote a comment on the :preprocess gist

thheller21:07:20

:foreign-libs could provide that mapping easily, just would need to do the same trick I do in shadow.cljs. ship a .js file that goog.provide("some.alias"); some.alias = React;

anmonteiro21:07:21

@juhoteperi updated the patch with an example for the single export case

anmonteiro21:07:10

basically this just works:

(ns foo.core
  (:require "create-react-class"))

(println create-react-class)

thheller21:07:27

@anmonteiro don’t do that, that should never be allowed

anmonteiro21:07:31

now if you have stuff like "react-dom/server" you’ll need to alias

anmonteiro21:07:50

we already do that

anmonteiro21:07:00

just not for string requires (because we don’t support them yet 🙂 )

thheller21:07:33

well we also warn on single segment namespaces for a reason

thheller21:07:50

ah well guess its an alias so it doesn’t matter much

anmonteiro21:07:02

this is not a global name

anmonteiro21:07:15

it’s just a way we resolve variables inside the namespace