Fork me on GitHub
#cljs-dev
<
2017-07-10
>
juhoteperi00:07:57

I have Reagent working with string requires, which use Cljsjs code through generated wrapper Closure modules:

❯ cat outsite/public/js/out/globalmodules/cljsjs/create-react-class/development/create-react-class.inc.js
goog.provide("globalmodule$create$react$class");
goog.require("globalmodule$react");
globalmodule$create$react$class = window.createReactClass;

juhoteperi00:07:29

{:file "cljsjs/create-react-class/development/create-react-class.inc.js"
                 :provides ["create-react-class"]
                 :requires ["react"]
                 :js-global "createReactClass"
                 :file-min "cljsjs/create-react-class/production/create-react-class.min.inc.js"}
^ if foreign lib has :js-global option, it is used to generate that Closure module

anmonteiro00:07:00

@juhoteperi is the purpose of that maintaining backwards compat?

anmonteiro00:07:11

or just reusing what CLJSJS has?

juhoteperi00:07:44

I guess it can be thought as backwards compat

juhoteperi00:07:59

Adapting to use npm-deps will take time (and there will be some that won't use it, I think), and it will be slow if libraries have to choose between using npm-deps and old way (cljsjs)

juhoteperi00:07:21

This way the libraries can use the new requires and it will work with both npm-deps and (updated) cljsjs packages

juhoteperi00:07:40

It should be also possible to write similar wrappers for Node target

juhoteperi00:07:56

Then libraries could just use the same requires everywhere

anmonteiro00:07:44

:npm-deps is not going to be supported under Node

juhoteperi00:07:21

String requires can be used without npm-deps (with my changes)

juhoteperi00:07:31

We could just add code which turns (:require ["react-dom/server"]) into wrapper with globalmodules$react_dom$server = require("react-dom/server"); when using Node target

juhoteperi01:07:59

I'll write notes tomorrow about why I want this feature and about the implementation

thheller08:07:10

it works really well for node so good that you are working on that

thheller08:07:37

I was really disappointed at the first implementation of string requires for CLJS

dnolen11:07:00

@juhoteperi the Node.js parts just don’t seem high priority to me - if there’s something about this that will ease CLJSJS transition then that’s far higher priority.

dnolen11:07:09

re: :js-global is this actually practical? will a JS lib necessarily only export one global?

juhoteperi11:07:51

As far as I can see, using npm-deps and string requires like they are implemented now, would break Reagent in Node

juhoteperi11:07:08

So while CLJSJS is first priority for me, I think Node support is also important

dnolen11:07:15

I just think it can be handled separately

juhoteperi11:07:44

I'll write notes about this and we can see what other solutions we can come up

dnolen11:07:16

to me string based require is still right now only about :npm-deps

dnolen11:07:34

and we’re only doing it to get around the fact that we don’t have #|| literal in Clojure

juhoteperi11:07:09

People won't use :npm-deps if there is no library support, and libraries won't support if it means breaking all previous configurations and no support for Node etc.

dnolen11:07:59

I disagree that a large population of people care about their build working under Node if they are going :npm-deps

dnolen11:07:19

nearly all projects I’ve encountered are using Node.js for light testing at most

dnolen11:07:40

anything serious is using phantom or slimer

dnolen12:07:31

in anycase - I’m not against it working - but prioritizing is important

dnolen12:07:48

let’s get one working case solid then consider the other one

dnolen12:07:06

I don’t really see any danger here in making choices that will make Node.js case difficult

juhoteperi12:07:38

Reagent has support for prerendering using Node, and I wouldn't want to break it even if it is not widely used

dnolen12:07:15

you can pre-render with whatever you want

dnolen12:07:29

it’s just not a real blocker is all I’m saying

dnolen12:07:04

the other thing I’m not even particularly convinced that string based require for signaling Node.js stuff is a good idea

dnolen12:07:49

if we had #|| reader literal so we could do #|react/dom-server| and there just no way we would think to use that as Node.js target special case

dnolen12:07:30

I would suggest that whatever we want to do actually address Node.js usage need more hammock time than quickly piggiebacking on string based require

dnolen12:07:41

@juhoteperi another thought since @anmonteiro has already done a ton of work to index node_modules I don’t see why string based requires even needs to be involved here

rauh12:07:38

I haven't really followed the module splitting but quick Q: Does :verbose give some debug information about how the splits were done? Like when there were parent namespaces that were picked. (etc?)

dnolen12:07:06

feel free to add feedback to the PR

dnolen12:07:42

@juhoteperi instead we can use the JS index to figure out how we should go about requiring something

rauh12:07:57

Awesome! I always like it when compilers tools are verbose. The more info the better.

dnolen12:07:23

yeah the more I think about it :nodejs target + node_modules indexing seems cleaner

dnolen12:07:39

and string based require means only one thing - not a valid Clojure symbol

juhoteperi12:07:09

Yes indexing seems fine solution for that

dnolen12:07:40

it does means how we currently do it needs to be reworked - instead of the missing-js pass thing we need to know the whole index same as we do for Closure compatible libs on the classpath

juhoteperi12:07:38

Hmm, right currently node module index only includes those files that are used, and that is based on :npm-deps and missing modules. Alternative would be to index everything, and then only process those modules that are really required by Cljs sources.

dnolen12:07:13

I think we should index everything

dnolen12:07:21

it makes everything else simpler afterwards

martinklepsch12:07:38

the static resolve, how does it work? is that like a static map injected into the build?

juhoteperi12:07:08

Yes, I agree. I find the missing-js- modules.. quite backwards, would be simpler to check if something is js lib than presume it is because it doesn't already exist.

dnolen12:07:13

@martinklepsch it just checks if some value exists, if it does return a var

dnolen12:07:40

@juhoteperi yes I didn’t love it - but it’s now clear why it’s a suboptimal approach

dnolen12:07:10

so this I’m 100% OK with, index node_modules and use this to add Node.js enhancements

juhoteperi12:07:27

Anyway, my main priority is Cljsjs-style foreign libs, and Npm is an afterthought. Though changing the indexing will help with Cljsjs-style foreign libs also.

juhoteperi12:07:14

One other thing I was thinking about: How hard/what would it require to get Closure-lib understand Foreign-libs? E.g. goog.require("cljsjs.react"); from Closure lib.

dnolen12:07:16

@martinklepsch we have a Var type

dnolen12:07:52

@juhoteperi the problem is that the thing being required needs to look like a Closure lib

dnolen12:07:14

i.e. needs goog.provides

juhoteperi13:07:58

Changing the indexing will probably remove also any need for special handling of string require symbols. Strings only need to be used when the name has "/" but the logic to resolve the module is same for symbols and strings.

juhoteperi13:07:24

Well, I guess the logic is the same even now but string requires are used to select what Node modules to index.

thheller13:07:39

FWIW indexing ALL node_modules is not a path I’d go down

thheller13:07:55

well I went and quickly turned around

thheller13:07:12

far too many things related to “dev-dependencies”

dnolen13:07:26

why does that matter?

thheller13:07:51

well have you looked at a node_modules folder recently that had babel in it or so?

thheller13:07:07

you can index all you like .. its just going to take time

dnolen13:07:15

@juhoteperi yes, getting rid of the connection w/ :npm-deps would be desirable.

thheller13:07:17

and 90% of the info you don’t need

dnolen13:07:36

95% of Google Closure you don’t need either and we index that

dnolen13:07:02

we also index every Google Closure compatible lib on the classpath

dnolen13:07:08

you probably don’t need those either

dnolen13:07:23

anyways - just don’t see how this point is relevant

thheller13:07:25

find . -type f -name "*.js" | wc -l
   12697

thheller13:07:40

just saying …

thheller13:07:07

you can do whatever .. it was just a point I feedback

dnolen13:07:10

it just doesn’t seem important

thheller13:07:13

something I learned while messing with this myself

dnolen13:07:25

the number of files is way less useful information than - “it took 500ms to index 12697 files”

thheller13:07:40

1ms per file on average

dnolen13:07:37

12 seconds for people who just want this feature doesn’t seem like a big deal to me

thheller13:07:52

but as I said you don’t need to follow everything so it is fine

dnolen13:07:55

also for in many cases I suspect just indexing the top level is sufficient

juhoteperi13:07:59

Hm deps for them also need to be indexes for closure

juhoteperi13:07:25

But I think it should be posibble

dnolen13:07:33

@juhoteperi so are you thinking about prepending the provides/requires ? Google Closure needs to see those - this didn’t seem that attractive to me

dnolen13:07:48

(or do you have something else in mind?)

juhoteperi13:07:25

to the foreign-libs? No I doubt that would be a good idea.

juhoteperi13:07:15

But when testing :js-global it would have been usefulm, as it generates Closure modules that need to depend on the foreing-libs

juhoteperi14:07:27

My current code for :js-global is just a proof-of-concept, there are some parts (like using module names in js-dependencies to modify :requires) that are bad ideas

juhoteperi14:07:33

Unfortunately changing the modules_deps.js to index all files is not super easy, as module-deps works by indexing the modules used in given JS input file

dnolen14:07:05

@juhoteperi yeah we should get @anmonteiro’s input here when he’s around

juhoteperi14:07:34

Yeah. In the meanwhile I'm checking what Closure compiler does. I think they might index the node modules lazily.

dnolen14:07:39

@juhoteperi I don’t see any issues with indexing the top level as a first step

dnolen14:07:04

what António has done should be easy to modify to accomplish just that - it’s just about computing what the top level :provides

dnolen14:07:08

re: :js-global it would be useful to understand at a higher level what you are trying to accomplish - i.e. state the problem so that your current line of thinking is simple to grasp and open up ideas about alternatives.

dnolen14:07:01

I mean at high level I see you want to permit fluidity in :npm-deps cljsjs choice - but need to understand what the obstacles (problems) are

rauh14:07:32

Just accidentally wrote (true x) instead of (true? x), got a runtime error. Should this be caught by CLJS analyzer?

dnolen14:07:09

@rauh it could, but not a priority

anmonteiro15:07:08

@dnolen @juhoteperi 1. should be easy to index the entire Node modules folder with some exceptions (see 2.), but I don't exactly understand what it would provide? 2. The caveat is things of the form react-dom/server that are not reachable from the top level and why I went for the missing-js-modules approach

juhoteperi15:07:35

"index the entire Node modules" -> all JS files, including server.js

dnolen15:07:58

@anmonteiro if we have a master index of all js deps we can know if they are :goog or :node

dnolen15:07:22

if :target is :nodejs and the dep is :node we can emit a js/require for that lib

anmonteiro15:07:34

Ah, and make Node support work with npm-deps?

dnolen15:07:37

this would solve the browser vs. Node.js issue

anmonteiro15:07:03

We wouldn't process the modules in that case

dnolen15:07:07

that’s right

dnolen15:07:19

the point would be to hide the user from all these details

anmonteiro15:07:47

Yeah this sounds like a good path forward. I can see what it takes to index all the Node stuff

dnolen15:07:22

@anmonteiro it would be worth considering how to avoid indexing everything

dnolen15:07:38

like transitive deps - I don’t see why we would care about that

anmonteiro15:07:15

Will we still have :npm-deps?

dnolen15:07:36

it’s still useful yes

anmonteiro15:07:43

Then I can just index everything inside those folders only

dnolen15:07:03

basically we splitting all these concerns apart

anmonteiro15:07:15

If you wanna require a transitive dep of React, you have to add it to npm-deps

dnolen15:07:21

1) :npm-deps is just about declaring deps to NPM stuff

anmonteiro15:07:21

Or does that not work?

dnolen15:07:32

2) string based require is just about things that can’t be symbols

anmonteiro15:07:45

Oh ok that won't work then

dnolen15:07:45

3) whether npm modules get compiled and whether we emit goog.require or js/require or not has to do with :target

anmonteiro15:07:24

I might divide this experiment into milestones to prove it's possible. e.g.: 1. Index only stuff reachable from the top level 2. Naively index everything 3. Perhaps experiment with a depth option 4. Clean up & dedupe

dnolen15:07:45

sounds reasonable

anmonteiro17:07:45

hrm didn’t know String.prototype.endsWith was ES6 only

anmonteiro17:07:00

submitted https://dev.clojure.org/jira/browse/CLJS-2208 fixing an occurrence in module_deps.js

dnolen17:07:45

I’m thinking since the dev work around the other posts are still very much in flight, maybe my code splitting post should go out first?

dnolen17:07:09

the only thing missing is an additional PR for the guide, but that’s mostly going to be a copy and paste affair

mfikes18:07:56

Do You think what we've been discussing with cljs-oss is “snapshot testing” mentioned by James here? https://twitter.com/jlongster/status/884479533692399618 (Wondering if that is a common name for the pattern.)

mfikes18:07:19

Pertinent aspect: “cargotest is a small tool that runs the test suite of several significant out-of-tree Rust projects. The projects are chosen to have a wide variety of dependencies to maximize the chances of detecting type system regressions through build failures.”

dnolen18:07:00

hrm that’s interesting

mfikes18:07:32

Thought you made this stuff up @dnolen :)

dnolen18:07:12

heh no, I just thought the Clojure build matrix approach makes sense with outside projects, especially the more popular ones

mfikes18:07:42

Yep. I thought it was novel. :)

mfikes18:07:15

Wow, it seems they test the entire corpus of open source Rust code with each PR.

bronsa18:07:27

heh, that's similar to how @andy.fingerhut made sure I didn't introduce subtle edge cases in tools.analyzer every new release years ago, https://github.com/jonase/eastwood/tree/master/crucible

dnolen18:07:00

it’s an interesting idea

bronsa18:07:33

empirically it's a better test suite than any number of unit tests you could ever come up with, IME

bronsa18:07:36

especially for compilers

mfikes18:07:26

I've always wanted to run the bajillions of successful 4clojure solutions through ClojureScript tô see what works, barring Java interop issues.

mfikes18:07:37

Also have been wondering the same with Spec to generate code. Both ideas are that ClojureScript uses Clojure as a reference for correctness

darwin18:07:30

I made quite nice progress on https://github.com/cljs-oss/canary, would be great if someone could look and stop me if this not the best idea 🙂

darwin18:07:54

people will be writing clj functions or shell script (if preferred): https://github.com/cljs-oss/canary/tree/master/runner/src/cljs_oss/projects

darwin18:07:18

the tool will run them in parallel and report results back: https://asciinema.org/a/6GbGI9swdQez8lKGSahun4Msm

dnolen18:07:41

@bronsa I agree it’s useful, but like the post alludes it’s not the answer by itself by any means

bronsa19:07:00

certainly not

dnolen19:07:12

it would be nice if they talked about how long this takes, how many machines etc. it seems like the most costly and slowest form of testing

dnolen19:07:37

also the noisiest

anmonteiro19:07:14

@dnolen the code splitting article looks great! I just left one note about a claim that I don’t believe is entirely accurate

dnolen19:07:00

@anmonteiro yeah was just looking at that I don’t see how the loader is important wrt. to how vendorization requires you to write useless code?

anmonteiro19:07:08

definitely true for the plugins part, but we’re kinda still managing splits via the source code with cljs-loader and resolve

dnolen19:07:36

I’m not talking about managing the splits, rather how to define them

anmonteiro19:07:40

Webpack also allows you to add several entrypoints

dnolen19:07:48

yeah I know about

anmonteiro19:07:50

just like our :modules declaration

dnolen19:07:25

I’m simply pointing out what goes into a split isn’t expressed in the source and doesn’t require a plugin

dnolen19:07:40

(but I agree that might not be clear and would happy to address that)

anmonteiro19:07:49

I see. Now I get it but it isn’t clear

dnolen19:07:38

that’s good feedback will fix

dnolen19:07:21

Webpack kind of complects splitting and loading - and I should be clear what I’m talking about here

dnolen21:07:05

@anmonteiro I tweaked the language about Webpack comparison to clarify it’s only about split definition (not loading)

dnolen21:07:17

@rauh I now link to the new guide which sets :verbose.

anmonteiro21:07:33

much clearer

dnolen21:07:43

cool thanks!

dnolen21:07:47

@juhoteperi left some feedback, the technical parts are pretty good, but I think the post needs more setup / marketing mojo 🙂

juhoteperi21:07:58

Yeah definitely

anmonteiro21:07:23

@dnolen there’s a weird issue on the code snippets in the module loading blog post

anmonteiro21:07:30

on hover, the word clojure appears

anmonteiro21:07:32

can you repro?

dnolen21:07:50

@anmonteiro yes it’s very strange, haven’t been able to figure that

juhoteperi21:07:13

https://clojurescript.org/css/asciidoctor-mod.css .listingblock:hover code.clojure:before { content: "clojure"; }

juhoteperi21:07:56

And there is another rule above that, which should position this text absolute: .listingblock:hover code[class*=" language-"]:before { text-transform: uppercase; font-size: 0.9em; color: #999; position: absolute; top: 0.375em; right: 0.375em; }

juhoteperi21:07:11

But I think that is not matched because the classes are not named language-

dnolen21:07:48

@juhoteperi hrm so what change should I make? I have access to the styles

juhoteperi21:07:38

Why do the code blocks on e.g. guides get language-clojure class instead of clojure

dnolen21:07:57

hrm I don’t know?

anmonteiro21:07:21

@dnolen maybe use [source,clojure] instead of [code,clojure]

dnolen21:07:30

good point

anmonteiro21:07:00

but the same is happening on the quick start with XML and you’re using source there I think

dnolen21:07:16

oh I use ` instead of ---- I think?

dnolen21:07:18

hrm don’t think so

dnolen21:07:26

quick start uses ----

dnolen21:07:35

so I think we should probably hack this via CSS if possible

dnolen21:07:36

@juhoteperi I just removed all those rules

dnolen21:07:54

hover issue seems resolved

dnolen21:07:06

just waiting for the link to the guide to update

dnolen21:07:34

ok that seems fixed now to, if anybody sees anything else strange let me know

mfikes22:07:09

Wow, very nice post! Also, very nice feature.

dnolen22:07:00

@mfikes yes though I think the wow factor will go up significantly when we point out that we can apply the same trick to ES6 sources w/ static import/exports in our builds 🙂

dnolen22:07:12

I took a look at https://rollupjs.org today

dnolen22:07:36

as I expected JS people are finally seeing the light about DCE, static import/export

anmonteiro22:07:07

yeah, I use Rollup to bundle Lumo’s JS

anmonteiro22:07:40

besides doing static import/export analysis it also does scope hoisting, i.e. doesn’t create a function scope for every module

dnolen22:07:35

I think we should refer back to JS tools like this consistently in our coming posts to give people the necessary reference points / context

anmonteiro22:07:39

I’m going to try and edit the JS modules blog post today

anmonteiro22:07:51

I’ll keep that in mind

dnolen22:07:36

looks like Google Closure Compiler started taking advantage of multiple threads recently https://github.com/google/closure-compiler/blob/master/src/com/google/javascript/jscomp/PrebuildAst.java#L58

dnolen22:07:02

ran into this when fixing our ES6 module processing pass

anmonteiro23:07:22

I just hope that translates well to JS with GWT

anmonteiro23:07:12

@dnolen btw do you know if Closure has public JavaDoc somewhere?

dnolen23:07:44

hrm not that I’m aware of

dnolen23:07:17

probably worth running IntelliJ just for navigating the source 🙂

anmonteiro23:07:08

probably possible to generate

anmonteiro23:07:36

mvn javadoc:javadoc works

anmonteiro23:07:40

not sure why they don’t publish it

anmonteiro23:07:58

(had to reset to a working version, it’s currently broken on master)