Fork me on GitHub
#cljs-dev
<
2020-06-04
>
dnolen13:06:17

@bhauman hrm, I can't reproduce https://clojure.atlassian.net/browse/CLJS-3258 with either master or 773

dnolen13:06:41

maybe you should add -Srepro to your clj command?

dnolen13:06:10

I couldn't reproduce this one either https://clojure.atlassian.net/browse/CLJS-3253, I made something minimal no warnings for me

dnolen13:06:02

both of seem like issues that were fixed sometime ago - like you're not running the version of ClojureScript you think you are - but that may be a bad hunch

dnolen13:06:34

@dominicm I don't think you added whether this ticket was a regression or not https://clojure.atlassian.net/browse/CLJS-3255?

bhauman14:06:58

@dnolen hmmm thats really strange about https://clojure.atlassian.net/browse/CLJS-3258 , as it was reported to me, and then I reproduced it twice in 773 in two separate builds

bhauman14:06:38

did you look at the compiler output?

dnolen14:06:53

but you know what it might be cache bug?

dnolen14:06:04

delete ~/.cljs

dnolen14:06:06

and try again

dnolen14:06:49

probably that but please confirm

dnolen14:06:56

it's because :target will always be :nodejs

dnolen14:06:03

there's nothing to differentiate

dominicm14:06:38

@dnolen every version I checked was broken, I got a different error at some point. I think it was arity, but not sure.

dnolen14:06:58

@dominicm right just not supported ok, going to clarify

dominicm14:06:23

Fwiw, it works for advanced, but not dev.

bhauman14:06:56

yeah it works, and its using the global “React”

bhauman14:06:13

so its a cache bug

bhauman14:06:57

well that’s really go to know

bhauman14:06:22

I thought I tried :aot-cache false in my testing but oh well

bhauman14:06:38

cool will do

bhauman14:06:23

@dnolen I have to populate the cache with something bad first right?

bhauman14:06:00

How do I get to that place?

dnolen14:06:26

just switch between :target :bundle and :target :nodejs

dnolen14:06:40

it should always work

dnolen14:06:40

I mean you can try to run it but it's really not necessary

dnolen14:06:42

just check the output

bhauman14:06:44

ok it works

bhauman14:06:09

actually not

bhauman14:06:37

the bug occurs when you change from one kind of dep to another

bhauman14:06:11

I.E. when you are shifting between cljsjs libs to node libs

bhauman14:06:30

ah that’s hard

bhauman14:06:00

and it will be common when folks are porting their apps

bhauman14:06:33

the cached version of reagent will be depending on the global react

bhauman14:06:08

or vice versa

bhauman14:06:57

so I can reproduce this by switching the type (cljsjs or node) of dependency for a given library

dnolen15:06:09

@bhauman master should address this problem - look at the output

dnolen15:06:22

:nodejs-rt should be in the first line now where it wasn't before

bhauman15:06:30

I saw that

dnolen15:06:48

but when you switch everything gets recompiled

dnolen15:06:10

but maybe cache-keys lemme check

dnolen15:06:30

@bhauman nope - it should part of the cache key now

dnolen15:06:37

so I do not understand what you are saying yet

dnolen15:06:56

there's no such thing as "cached reagent"

bhauman15:06:57

OK if I have react installed node modules

dnolen15:06:09

that's not cached according to all build affecting options

bhauman15:06:10

and the I remove react from node_modules

bhauman15:06:14

so this isn’t reflected in the build options

bhauman15:06:21

I’m not changing build options

bhauman15:06:12

its affected by the presence or absence of a node_module

bhauman15:06:00

in one case reagent is compiled referencing a node_module, and in another it is compiled referencing a global React

bhauman15:06:30

so it gets cached one way or the other

bhauman15:06:55

even though they both have nodejs-rt false

dnolen15:06:42

the waters are getting muddied right now by this description, let describe how I think it can actually go wrong.

dnolen15:06:55

here's what I think will happen normally

dnolen15:06:11

Scenario A: User has built Reagent w/ React CLJSJS w/o :target :bundle

dnolen15:06:39

1. The switch to :target :bundle (and installed React into node_modules everything will be recompiled everything will be cached on a new path

dnolen15:06:13

@bhauman ^ do you agree this is a happy path for most people?

bhauman15:06:40

I’m trying to think of the best way to communicate this

dnolen15:06:54

let's just go w/ what I'm saying

dnolen15:06:08

is this the happy path - yes or no?

dnolen15:06:21

then we can describe things that could wrong because of missed steps

bhauman15:06:45

I think the normal case is that they switch to bundle and build

bhauman15:06:06

without adding the node_modules

dnolen15:06:14

I'm just trying to describe a minimal reproducer, let's just ignore all the things that can go wrong

dnolen15:06:25

I understand you're interested in the UX but we only need a reproducer

dnolen15:06:01

ok - so the above scenario works

dnolen15:06:43

2. what happens is that if the user never runs the install step - they will cache something that points to CLJSJS

bhauman15:06:47

I’m actually having to move a TV, I’ll be back

bhauman16:06:21

sorry bout that, ping me when you’re ready 🙂

dnolen16:06:51

do you agree about 2. ?

bhauman16:06:29

yes that can happen 🙂

dnolen16:06:12

do you agree that all other cases are really some form of 2. ?

bhauman16:06:52

yes caching something under :nodejs-rt false

dnolen16:06:11

the order may change what gets cached - but in the end it doesn't really matter, they all look like 2.

bhauman16:06:11

or just caching something

bhauman16:06:32

well they can cache something that points to CLJSJS or a node modules require

bhauman16:06:42

that’s a more general 2

bhauman16:06:48

more specifically a library that exists outside of the project like reagent gets cached that way

dnolen16:06:57

so here's the thing - I don't actually see any good answers to this problem yet - let's point out some basic assumptions

dnolen16:06:34

1. Realated, AOT Cache can't understand macro time configuration because it is file specific, not build specific

dnolen16:06:54

2. AOT Cache caches along build configuration computed path only, no file specific stuff as stated in 1.

bhauman16:06:27

I understand that

dnolen16:06:31

3. Whether we use node module or CLJSJS is in fact exactly the same same problem as 1., a file specific thing

dnolen16:06:57

4. it's allowed that what's in node_modules is never specified

bhauman16:06:18

cool you understand the problem then

bhauman16:06:39

I totally get that it may not be fixable

dnolen16:06:19

well it points a bunch of prior choices that make it appear incompatible with a simple caching strategy

dnolen16:06:28

one alternative is validation to prevent cache corruption

dnolen16:06:42

i.e. break 4. or at least supply a flag to break 4.

dnolen16:06:29

that is you can't use something not both in :npm-deps and in node_modules

bhauman16:06:44

doesn’t sound ideal either

bhauman16:06:03

in practice there is a rough patch when folks are switching over

dnolen16:06:04

let's be clear to

dnolen16:06:12

this only affects any tool default to :aot-cache true

dnolen16:06:16

which is not true by default

bhauman16:06:19

and are unaware of what libraries depend on what

dnolen16:06:34

you don't need to know what depends on what

dnolen16:06:41

upstream :npm-deps is a thing

bhauman16:06:53

cljs.main defaults to :aot-cache true

dnolen16:06:09

no burden on the user other than installing the deps and specifying their own

dnolen16:06:49

yes cljs.main specifically designed for people who are going to audit their deps because we don't care about the macro build time problem

dnolen16:06:34

so we're already just saying you're on own - I don't really care that much about this caching problem and order stuff because it's a one time thing

dnolen16:06:15

but I do care that downstream tooling can provide better UX if that's desired

dnolen16:06:12

but breaking 4. is not as bad it sounds - it's more disciplined and would in fact catch simple mistakes

dnolen17:06:20

really 4 is just :enforce-deps - either :install-deps is true, or the libs are present at the top level

dnolen17:06:52

this means it's not possible to use Reagent w/o installing the deps first - because you're saying they must come from there not elsewhere

dnolen17:06:04

since node_modules wins over foreign libs

dnolen17:06:46

just noting that changing caching is quite unattractive because you would be generated useless cache files while you're changing your deps at the beginning of the transition

dnolen17:06:10

better to just turn it off during transition

bhauman17:06:06

the cache is not build dependent so this gets tricky

dnolen17:06:37

yes it doesn't know anything about builds - just build affecting options

dnolen17:06:41

because you want the cache to be available to all builds

dnolen17:06:32

again this is due to dependency ambiguity

dnolen17:06:51

which isn't generally true for Clojure(Script) stuff, just libs that don't enforce one or the other

dnolen17:06:10

so this once again the same problem rearing it's head

bhauman17:06:40

buts its only because the way of requiring the dependency is in the file itself

bhauman17:06:51

it its not abstracted

dnolen17:06:55

it has nothing to do with that

dnolen17:06:06

it's a dependency ambiguity plain and simple

bhauman17:06:06

OK bear with me

bhauman17:06:12

yes I hear that

dnolen17:06:15

you don't know know where the dependency is coming from

bhauman17:06:08

if the code for requiring the cljsjs dependency and the node dependency was rendered the same in the output this isn’t a problem

dnolen17:06:24

completely unrelated

dnolen17:06:36

think about a two Maven dependencies with the same package name

dnolen17:06:42

you don't know what you're going to cache

dnolen17:06:48

this happens with Clojure AOT

bhauman17:06:43

I’m saying that if this line

reagent.impl.component.global$module$react = goog.global["React"];

bhauman17:06:54

was the same for both node and cljsjs

bhauman17:06:04

then they are caching the same thing

dnolen17:06:08

I'm just pointing out you're talking about solutions

bhauman17:06:54

i get that you are addressing fundamentals

dnolen17:06:39

to your point - I just don't see anything fruitful around how we "link" the library

dnolen17:06:08

but maybe you see something, all ears

bhauman17:06:35

having external libs be linked in a similar manner is kinda interesting to me

dnolen17:06:02

sure but I don't see anything that would work?

bhauman17:06:34

well under bundle we have npm_deps, why not have cljsjs libs scoped to a JS object?

bhauman17:06:08

it kinda sucks that these things are global anyway

bhauman17:06:41

there are problems with backward compatibility here, which is a real practical problem

dnolen17:06:02

also load order w/ webpack

dnolen17:06:07

we are async webpack isn't

dnolen17:06:26

also maybe a node_module DCE issue?

dnolen17:06:05

anyways this would be moving around a lot of chairs

bhauman17:06:12

yeah nothing simple

dnolen17:06:25

so it's not very enticing

bhauman17:06:01

we couldn’t have the npm_deps.js inline the cljsjs lib could we?

bhauman17:06:20

nah load order

bhauman17:06:25

never mind

dnolen17:06:39

I think maybe load order isn't an issue

dnolen17:06:57

because webpack ensures everything is present - this works for the REPL

dnolen17:06:45

but for DCE and code splitting ... changing stuff is probably a bad idea

dnolen17:06:38

you really don't want foreign libs to go into one place for code splitting

dnolen17:06:26

for :bundle npm_deps.js is dev time only, so the door remains open for DCE improvements later

dnolen17:06:56

so there's just a bunch of considerations in the way of making this easy

dnolen17:06:59

only to make everything else hard

bhauman17:06:10

good points

dnolen17:06:15

one other option probably has it's own issues is emitting require for foreign libs

dnolen17:06:27

but then you need to configure your bundler so that's moving work around

dnolen17:06:28

I think the issue need more stewing

dnolen17:06:05

hrm though webpack can handle relative require so maybe no config

dnolen17:06:14

I think the blocker here is that many foreign libs are probably packaged expecting to be global?

dnolen17:06:44

fixing up a bunch of libraries when you're transitioning away doesn't sound very valuable

dnolen17:06:47

@juhoteperi I think worth pondering dropping the Reagent explicit dependency on cljsjs/react users can always add it

henryw37405:06:46

@dnolen I am really surprised by this. Non-npm users are supposed to manually maintain the cljsjs deps and versions of all the libs they use? non-npm is a significant proportion of users afaik why not fix this similarly to how shadow has done? ie new compiler opt something like :ignore-foreign-libs-in-deps . it could default to false

dnolen17:06:08

I think pointing to both things creates more problems than it's worth as the backlog will show

dnolen17:06:57

@bhauman I think the simplest answer is to just have libraries just choose one dependency

dnolen17:06:09

then the problem just goes away

juhoteperi19:06:08

@dnolen Yes, I've been planning to drop the cljsjs dependency

juhoteperi19:06:31

Probably before final 1.0 release

henryw37405:06:46

@dnolen I am really surprised by this. Non-npm users are supposed to manually maintain the cljsjs deps and versions of all the libs they use? non-npm is a significant proportion of users afaik why not fix this similarly to how shadow has done? ie new compiler opt something like :ignore-foreign-libs-in-deps . it could default to false