Fork me on GitHub
#shadow-cljs
<
2017-10-17
>
mhuebert00:10:12

could it have anything to do with direct vs. transitive dependencies?

thheller00:10:53

shouldn’t. might need some code I can look at.

thheller00:10:57

off to bed, gn8

mhuebert09:10:03

@thheller I think I have narrowed this down. when :lein true is set, shadow-cljs npm-deps does not appear to install transitive deps. I’ve pushed the config to /website in re-view: https://github.com/braintripping/re-view.git

mhuebert09:10:25

In the repo, :source-paths in shadow-cljs.edn is identical to :source-paths in project.clj, but commented out. I would expect there to be no difference between where :source-paths are specified when using :lein true. but if I run shadow-cljs npm-deps without specifying the deps in shadow-cljs.edn, they do not install. if I uncomment :source-paths in shadow-cljs.edn, then it works. (even if I keep :lein true)

thheller10:10:48

oh, yes. npm-deps does not use lein, so it doesn’t pickup the dependencies of that, since no :dependencies are listed in shadow-cljs.edn

thheller10:10:28

will fix that so it also uses lein

thheller10:10:31

how come you are sticking to lein? just because of the cursive integration or some other reasons?

thheller10:10:03

there is about a 10sec startup time penalty when using lein

mhuebert10:10:19

cursive yes.

mhuebert10:10:52

checkouts is also a minor thing. now i am switching to only source-paths, but then I have to remember to add :dependencies manually

thheller10:10:25

dependencies should work the same way as in lein

mhuebert10:10:41

I just mean using :source-paths means it doesn’t pick up the transitive deps

thheller10:10:55

so you keep the thing dependency but also include checkouts/thing/src path

thheller10:10:11

the src path will be picked over whats in the jar

thheller10:10:35

just need to lein install if you change the dependencies of checkout

thheller10:10:43

I think lein behaves the same way

thheller10:10:56

Colin said he is going to add support for deps.edn soon, when that comes I can take advantage of that too

mhuebert10:10:42

so, I think it should work now without any lein install-ing, to try shadow-cljs release browser in re_view/website, and you can observe that if componentDidUpdate is not listed in browser.txt, the build breaks. I am not sure why it is not being included in the auto externs

mhuebert10:10:33

shadow-cljs npm-deps first should work

mhuebert10:10:45

I see, they were added there by shadow 🙂

thheller10:10:01

yeah I npm install --save

mhuebert10:10:36

i think thats fine

thheller10:10:22

ok website build completed

mhuebert10:10:21

so i assume if you run shadow-cljs server and open the page, it will load fine. but if you comment componentDidUpdate in externs/browser.txt and build again, then there is an externs error

thheller10:10:52

hehe this ES6 generation is really slow …

thheller10:10:09

the faster version goes from about 500ms per file to 18ms

mhuebert10:10:27

so in target/shadow-cljs/builds/browser/release/externs.shadow.js I don’t see any lifecycle methods, but I do see isPureReactComponent, isValidElement, other React things

thheller10:10:48

all the component* fns are missing

mhuebert10:10:38

I use a lot of lifecycle methods so i don’t know why only this one is not working.

thheller10:10:02

I really need to fix this react workaround

thheller10:10:35

dev and release are both included 😞

mhuebert10:10:04

was react handled as a special case?

thheller10:10:42

noh, they are a special case

thheller10:10:57

'use strict';

if (process.env.NODE_ENV === 'production') {
  module.exports = require('./cjs/react.production.min.js');
} else {
  module.exports = require('./cjs/react.development.js');
}

thheller10:10:04

they do this nonsense

thheller10:10:21

never saw any other package do this

thheller10:10:10

my inspector which extracts the require calls to collect the dependencies

thheller10:10:20

does not cover conditional includes

mhuebert10:10:34

oh that does seem wrong

thheller10:10:35

which is why the :resolve {"react" ...} override must be done for react

mhuebert10:10:39

I hesitate to draw conclusions yet, because I don’t know why or if something is wrong somewhere 😉, but the shadow-cljs advanced build using these direct node_modules requires vs. my old webpack stuff has led to a smaller build, 950kb vs 1.4mb

thheller10:10:30

well if you included multiple separate bundles it should be smaller yes

thheller10:10:58

ok, figured out why the lifecycles are missing

thheller10:10:04

they are never actually assigned anywhere

thheller10:10:39

if (typeof instance.componentWillReceiveProps === 'function' && (oldProps !== newProps || oldContext !== newContext)) {
      callComponentWillReceiveProps(workInProgress, instance, newProps, newContext);
    }

thheller10:10:52

thats not something I can extract

mhuebert10:10:31

separately: in re_view/src/ the deps.cljs includes some :externs… are these not being picked up?

mhuebert10:10:11

or did I include them wrong somehow

thheller10:10:25

ah hmm, I might have deleted that part when I removed support for cljsjs

thheller10:10:39

externs should still be picked up though

thheller10:10:41

hmm nope, should be picked up

thheller10:10:43

I have an idea but I hope its not related

thheller10:10:04

haha the babel transform also bypasses all caching

thheller10:10:10

no wonder its so slow

thheller10:10:35

ok found it, the deps.cljs externs are not included

mhuebert10:10:15

re: earlier comment about react - do you mean that in this current advanced build, react + react-dom are being included twice?

thheller10:10:46

hmm yeah thats really a problem I need to solve, the problem really is that you are not supposed to include those externs

thheller10:10:04

if you include externs for react but not provide react

thheller10:10:09

and someone else does as well

thheller10:10:17

they will cause a conflict and closure will complain and fail

mhuebert10:10:48

so including react in :npm-deps doesn’t count as ‘providing’ react

thheller10:10:55

yes, react will be included twice. but release will remove the dev version so its not a big deal in release.

thheller10:10:12

npm provides react, you just depend on it

thheller10:10:49

I should just start a repo collecting externs for certain npm packages

thheller10:10:08

so I can include the react externs if you (:require ["react" ...])

thheller10:10:24

this automatic generation really isn’t working all too well 😞

mhuebert10:10:29

so even for this it would make sense to have a way to map from npm module names to externs files, that way there isn’t duplication

mhuebert10:10:50

if it was `:npm-externs {“react” “externs/react.ext.js”}

mhuebert10:10:19

then if multiple projects would declare them, one could de-dupe, without needing a global registry that is maintained

mhuebert10:10:38

i doubt it is particularly fun maintaining a thing like cljsjs 😕

thheller10:10:54

yeah but every lib depending on react would need to add :npm-externs

thheller10:10:10

yeah I really don’t want to maintain such a thing

thheller10:10:19

but its far easier since its just about externs

thheller10:10:32

not about repackaging whole npm packages

thheller11:10:35

externs is one thing cljsjs got right, its perfect for that

thheller11:10:54

but I want to get rid of these cljsjs/react dependencies 😞

mhuebert11:10:28

so closure compiler will complain if duplicate externs are provided?

thheller11:10:30

damn this automatic generation

mhuebert11:10:48

that is unfortunate

thheller11:10:24

and one other issue I forgot the name of

thheller11:10:38

problem is de-duping is not easy

mhuebert11:10:43

is the error from duplicate file content, or file path?

thheller11:10:53

duplicate var name

thheller11:10:16

if one extern declares var React; no other is allowed to do that

thheller11:10:38

so even if you de-dupe based on the file content hash

thheller11:10:54

there is no guarantee that they will have the same hash

thheller11:10:17

there really should be one “source of truth” for externs

thheller11:10:27

hmm where do you use componentDidUpdate? maybe we can solve this by annotating the code?

mhuebert11:10:50

even with source-maps I don’t seem to get pointed to direct usage

mhuebert11:10:53

maybe because of macros

thheller11:10:05

(.componentDidUpdate this)

thheller11:10:38

should work with (.componentDidUpdate ^js this)

thheller11:10:24

(fn [^react/SyntheticEvent e] I don’t think that is valid? should start with ^js/? i think

mhuebert11:10:32

but wouldn’t js/ imply that it is a global variable

mhuebert11:10:39

whereas here it is referred in as react

thheller11:10:20

no, tags are not resolved that way

thheller11:10:53

it checks if its a js-tag?, if so it uses the remainder to write externs

mhuebert11:10:10

oh i found the .componentDidUpdate

thheller11:10:15

(defn js-tag? [x]
  (and (symbol? x)
       (or (= 'js x)
           (= "js" (namespace x)))))

mhuebert11:10:16

ok, should be easy fix

thheller11:10:56

but for shadow-cljs all that matters is the ^js tag anyways, the type info is optional

mhuebert11:10:06

so is it possible to annotate that e is an instance of my-referred-react-thing.SyntheticEvent

thheller11:10:36

they are not resolved I think

thheller11:10:56

didn’t quite understand that part of the externs inference

thheller11:10:10

they really can’t be resolved

mhuebert11:10:34

tbh i viewed those annotations as a form of necessary but poorly understood voodoo

mhuebert11:10:03

and with enough of them, things would compile

thheller11:10:25

thats why I simplified to just ^js, without the type structure

thheller11:10:02

(defn my-fn [^js/Foo.Bar x]
  (let [z (.baz x)]
    (.-wozz z)))

mhuebert11:10:17

damn, (.componentDidUpdate ^js this) does not appear to have worked

thheller11:10:24

that would be totally accurate but who wants to write that

thheller11:10:11

(def kmap
  "Mapping of convenience keys to React lifecycle method keys."
  {:constructor             "constructor"
   :view/initial-state      "$initialState"
   :view/state              "$state"
   :view/did-catch          "componentDidCatch"
   :view/will-mount         "componentWillMount"
   :view/did-mount          "componentDidMount"
   :view/will-receive-props "componentWillReceiveProps"
   :view/will-receive-state "componentWillReceiveState"
   :view/should-update      "shouldComponentUpdate"
   :view/will-update        "componentWillUpdate"
   :view/did-update         "componentDidUpdate"
   :view/will-unmount       "componentWillUnmount"
   :view/render             "render"})

thheller11:10:28

is that used in a macro or so?

mhuebert11:10:09

no, that is used with a call to goog.object/set

mhuebert11:10:38

i don’t think that is the problem… I think it is line 100 of re-view.material.util

thheller11:10:44

hehe wow .. this babel thing is so super annoying

thheller11:10:53

I really need to fix that

mhuebert11:10:11

source-mapping is taking me all over the place but seems nowhere near the actual source of .componentDidUpdate

thheller11:10:20

can’t stand slow builds

thheller11:10:39

source mapping is currently completely broken when using JS deps

thheller11:10:01

unfortunate side effect of the hacking I had to do to get that working

mhuebert11:10:28

in the javascript source near the error, one can see get-element and other things that make me believe it’s L#100 of that file. plus it is the only place in the project that uses .componentDidUpdate as a host interop call

thheller11:10:14

pseudo-names should still help

thheller11:10:59

god dammit … I’m fixing this now

mhuebert11:10:06

i remember now the macro stuff i was thinking of. i allow views to have custom methods included in the map, like {:my-method ...}, the macro writes these in host interop syntax so that you can use them like (.myMethod the-component) and closure will rewrite it all and it will still work, without any annotations.

mhuebert11:10:39

but all the :view/* stuff goes through that kmap you saw and is never rewritten with host interop syntax

thheller11:10:21

(gobj/extend prototype (lifecycle-methods lifecycle-keys) this is most likely it

mhuebert11:10:56

L#196 of re-view.core

thheller11:10:56

re-view.core/class*

mhuebert11:10:33

(doto obj
   (gobj/set (get kmap method-k) (wrap-methods method-k method)))

thheller11:10:35

checking if the annotation makes it through

thheller11:10:53

gobj/set should be fine

mhuebert12:10:53

if i move the ^js annotation into the arglist, it seems to work:

(fn [{:keys [view/state get-element] :as ^js this}]
  ...
  (.componentDidUpdate this))
`

mhuebert12:10:33

can’t say I have ever understood exactly where these annotations are allowed to go. i recall quite a bit of trial and error when i first added them.

mhuebert12:10:58

but this is good, adding ^js there in the arglist fixes the .componentDidUpdate call and I tried adding a custom method to the component and calling it in the same place, also worked.

thheller12:10:21

(.componentDidUpdate ^js this) should work as well, not sure why it wouldn’t

thheller12:10:07

must fix the babel speed issue first

thheller12:10:22

can’t debug when I have to wait a minute in every test 😛

mhuebert12:10:54

i appreciate this lack of patience 🙂

thheller12:10:09

slow tools are the worst

thheller16:10:27

-> Babel transform: node_modules/@material/animation/index.js
<- Babel transform: node_modules/@material/animation/index.js  (629 ms)
-> Babel transform: node_modules/@material/base/foundation.js
<- Babel transform: node_modules/@material/base/foundation.js  (34 ms)
-> Babel transform: node_modules/@material/base/component.js
<- Babel transform: node_modules/@material/base/component.js  (59 ms)
-> Babel transform: node_modules/@material/ripple/adapter.js
<- Babel transform: node_modules/@material/ripple/adapter.js  (17 ms)
-> Babel transform: node_modules/@material/ripple/constants.js
<- Babel transform: node_modules/@material/ripple/constants.js  (13 ms)
-> Babel transform: node_modules/@material/ripple/util.js
<- Babel transform: node_modules/@material/ripple/util.js  (44 ms)
-> Babel transform: node_modules/@material/ripple/foundation.js
<- Babel transform: node_modules/@material/ripple/foundation.js  (172 ms)
-> Babel transform: node_modules/@material/ripple/index.js
<- Babel transform: node_modules/@material/ripple/index.js  (48 ms)
-> Babel transform: node_modules/@material/selection-control/index.js
<- Babel transform: node_modules/@material/selection-control/index.js  (9 ms)
-> Babel transform: node_modules/@material/checkbox/adapter.js
<- Babel transform: node_modules/@material/checkbox/adapter.js  (12 ms)
-> Babel transform: node_modules/@material/checkbox/constants.js
<- Babel transform: node_modules/@material/checkbox/constants.js  (6 ms)
-> Babel transform: node_modules/@material/checkbox/foundation.js
<- Babel transform: node_modules/@material/checkbox/foundation.js  (68 ms)
-> Babel transform: node_modules/@material/checkbox/index.js
<- Babel transform: node_modules/@material/checkbox/index.js  (55 ms)

thheller16:10:02

first one is always slow since it has to spin up the node process but after that its pretty smooth

thheller16:10:53

still need to fix that it bypasses the cache and always calls out to babel

mhuebert16:10:55

that’s doing the same transform as before?

thheller16:10:31

just not launching a new babel process per file 🙂

thheller16:10:54

still need to fix a few things, please test

thheller16:10:11

now requires that shadow-cljs is installed into the project

thheller16:10:26

will add a check into the script that enforces that later

thheller16:10:03

re-view website [:browser] Build completed. (334 files, 233 compiled, 0 warnings, 22.40s) down from 47sec

thheller16:10:06

decent start

thheller16:10:27

almost ready to debug the actual issue 😉

thheller16:10:59

btw I just saw this ^js/Element element (get-element this)

thheller16:10:13

you should be able to annotate get-element for this

thheller16:10:30

(defn get-element ^js/Element [args …])

thheller16:10:50

looking into the missing .componentDidUpdate now

thheller16:10:55

(defn thing [{:keys [foo] :as this}]
  (.componentDidUpdate ^js this))

thheller16:10:13

desctructuring seems to lose the tag

thheller17:10:37

did you do any comparisons yet?

mhuebert17:10:19

Not yet. There are a bunch of bugs I have to work out

mhuebert17:10:27

Unrelated to shadow

mhuebert17:10:09

But I am impressed it's all working, removed every foreign-lib I think

mhuebert17:10:45

Using Babel is a good path I think. Closure is a hard road at this point

mhuebert17:10:31

Whereas this is how most JS libs expect to be used

thheller17:10:15

yeah, at this point I see the closure stuff as an optional optimization

thheller17:10:30

if someone really wants to get their hands dirty they can do it

thheller17:10:48

but the default will work more reliably with acceptable results

thheller17:10:16

I did some comparisons with how webpack bundles code

thheller17:10:43

and we are about the same size with slightly better performance characteristics since not everything is wrapped into one big IIFE

thheller17:10:20

you are the first to use such a vast amount of JS deps I think

thheller17:10:24

I’m amazed that it works 🙂

thheller18:10:21

I think I figured out the (.componentDidUpdate ^js this) issue

thheller18:10:52

since this is a destructured binding it is already tagged as clj

thheller18:10:24

I do think that you should probably be still allowed to tag it though

thheller18:10:37

I hacked a fix, no idea if thats correct though

mhuebert19:10:56

advanced build appears to have worked with 2.0.22

mhuebert19:10:13

in ./editor, I have 2.0.22 in package.json, but when I run shadow-cljs server I see

mattpro:editor MattPro$ shadow-cljs server
shadow-cljs - config: /Users/MattPro/Documents/sites2017/maria/editor/shadow-cljs.edn version: 2.0.7

mhuebert20:10:53

maybe it was in project.clj in a dep

mhuebert20:10:32

yeah, now unless I use yarn run ... it appears to use my old global version instead of local npm version

mhuebert20:10:33

not a big deal but seems like a regression

thheller20:10:37

update the global install

thheller20:10:01

I changed the way the package is bundled, the old one doesn’t pick that up correctly

mhuebert21:10:06

ah ok. so after updating global install to 2.0.22, then it tracks the npm version?

thheller21:10:54

added a fix for the next version just in case

thheller21:10:49

the shadow-cljs script checks for <project>/node_modules/shadow-cljs/cli/lib.js which doesn’t exist anymore

thheller21:10:07

added the file back which then requires the correct one

thheller21:10:35

kinda don’t like the way the global install is handled

thheller21:10:57

thinking it might be better to just recommend using npx shadow-cljs or yarn shadow-cljs to run things

thheller21:10:41

we are at a point where the install really must be in the project, it was sort of optional before

thheller21:10:29

brain is too fried, just checking out your configs and setup

thheller21:10:01

you might consider getting rid of the :trusted build and just code-split the :live build. there might be tons of overlap … nevermind you are on two domains, so no sharing

thheller21:10:39

also (enable-console-print!) is never required, shadow-cljs injects that so you don’t have to 😉

mhuebert22:10:00

yeah, the :trusted build can be advanced compiled, whereas the :live build uses selfhost and so must remain :simple.

mhuebert22:10:30

when I run npm-deps I am seeing:

[:npm-deps-conflict "codemirror" :using :a]
[:a "5.30.0" #object[java.net.URL 0x7f3a9c11 "file:/Users/MattPro/Documents/sites2017/lark/tools/src/deps.cljs"]]
[:b "5.30.0" #object[java.net.URL 0x7f888518 "jar:file:/Users/MattPro/.m2/repository/lark/tools/0.1.5/tools-0.1.5.jar!/deps.cljs"]]
I think because I am using these extra source-paths in lieu of checkouts