Fork me on GitHub
#shadow-cljs
<
2019-11-07
>
lilactown01:11:29

After thinking about the react refresh problem some more, I think the best thing to try right now is to add the ability to reload all dependent ns

lilactown01:11:24

thheller, would you be willing to help me if I tried to implement that in shadow-cljs to try it out?

lilactown01:11:56

I'll try and write up a clearer rationale for it

thheller14:11:32

and commenting out that check

thheller14:11:45

so (and #_(contains? compiled resource-id)

thheller14:11:10

(I think that should be enough)

thheller14:11:29

it'll probably make reload very slow though

lilactown17:11:32

it did reload the namespaces, but in my simple test it also reloaded React et. al. which broke the app

lilactown17:11:52

quick question: what's the :always-load bit there? is that something I can annotate an ns with?

thheller17:11:01

@lilactown the other case in the and must remain

thheller17:11:09

only comment out the compiled check

thheller17:11:08

hmm or now that I think about it the npm sources won't have the :from-jar flag

thheller17:11:41

so you can add another filter so it filters out all (str/starts-with? (str ns) "module$node_modules")

lilactown17:11:28

that will work for my test, but might that break hot loading of npm deps after (re)installation?

thheller17:11:16

well whatever it is the "logic" would need to happen in exactly that place

thheller17:11:25

there is nothing to change server side

thheller17:11:41

(except maybe make more data available)

thheller17:11:41

but if you checkout the msg data the handle-build-complete fn receives you should find everything you could possibly need

thheller17:11:36

don't really know why re-declaring a file plays a role in your setup though. it is going to be too slow for any practical uses anyways

lilactown17:11:21

the way react refresh is modeled, your macro outputs a call to a register function with a stable ID and a component. when you call register with a new component (not === to the last one), it marks it as dirty in the react refresh runtime and tells the react reconciler to re-render it or re-mount it

thheller17:11:45

yes, that part I understand

thheller17:11:05

but why is it necessary to replace all of them?

lilactown17:11:19

in the case where you update a namespace with a function or piece of data that components depend on, but isn’t a component itself, the dependent components won’t be refreshed

thheller17:11:59

yes, but that wouldn't be the case for regular JS either?

thheller17:11:12

or does it actually just reload all the code?

lilactown17:11:53

yes, it is. the current strategy they suggest to people implementing this in webpack, rollup, etc. is: - if a module exports only components use react refresh - if a module exports anything other than a component, reload the page

thheller17:11:49

that isn't the problem you are solving

thheller17:11:58

say my "module" only exports components

thheller17:11:07

but it is still nested

thheller17:11:13

so a -> b -> c

thheller17:11:16

c is reloaded

thheller17:11:19

a + b are not

thheller17:11:53

how does refresh do its thing to re-render c? assuming that hte problem you are trying to solve is that starting from a short circuits since it wasn't changed

lilactown17:11:04

in that case it’s fine, because a + b don’t need to be reloaded

thheller17:11:24

so why are you trying to solve the problem by reloading a+b?

lilactown17:11:48

because if c isn’t a component, then the components that use it to implement some behavior aren’t reloaded

thheller17:11:29

we are not talking about that case

thheller17:11:48

(which would be a full reload in react-refresh if I understand correctly)

lilactown17:11:07

I guess I should back up

lilactown17:11:25

I think the heuristic they use sucks

lilactown17:11:47

it means that if your module contains anything other than components, can’t use hot reloading; reload the whole app!

thheller17:11:00

yes, just forget about what they are doing

thheller17:11:23

the problem as I understand it is that doing the regular CLJS reload we have today is losing useState hook data (and others)

lilactown17:11:47

right, if you call React.render again you lose all state and re-mount everything

thheller17:11:50

which react-refresh does some "magic" to preserve

thheller17:11:19

so the problem to solve is how to hook into that part only right?

thheller17:11:31

don't care about the other nonsense

lilactown17:11:07

I have the simple case working where when you edit a component, it refreshes correctly using the react-refresh stuff. the “magic” works

lilactown17:11:33

the case I’m working on now is how to handle the case where in webpack, rollup, etc. they decide to bail and reload the page

lilactown17:11:46

where in the React component tree, you have:

<A>
  <B />   <---- uses `moduleC.foo`
</A>
when moduleC gets reloaded, but the module that contains components A and B are not reloaded, then B will not be refreshed

thheller17:11:43

so what you are trying to solve is "shouldComponentUpdate" deciding to skip updates?

lilactown17:11:59

no, I don’t think it has anything to do with shouldComponentUpdate

thheller17:11:48

hmm? but if you render from the root (as regular CLJS does) then it'll eventually reach the new moduleC.foo?

thheller18:11:07

unless it stops early because some component said "I don't need to render"

lilactown18:11:49

ah yes, another strategy that doesn’t use react-refresh might be to force a re-render from the root

lilactown18:11:14

react-refresh does some other magic that makes it so if you’ve changed a component in a certain way, it will re-mount that component, and it handles hooks in slightly different ways like restarting effects but keeps state

lilactown18:11:18

the example I created displays the problem I’m trying to solve: https://github.com/Lokeh/react-hmr-cljs/ I was actually able to get the behavior I want in that case by annotating the core ns as :dev/always

thheller18:11:31

wouldn't you get the same by just calling (start) after the (react-refresh/reload)?

lilactown18:11:06

that would re-mount the whole app, losing all state

lilactown18:11:09

I’m not sure it will refresh child components as well, so just marking the top-level ns as :dev/always might not be enough. that’s why I’m experimenting if I can reload all dependent namespaces

lilactown18:11:18

the other option I think is we can do something similar to webpack, which is to apply some heuristic to see if a namespace is “safe” to refresh and if not, remount everything via React.render

lilactown18:11:47

that’s more problematic for Clojure because everything is public by default… so just checking to see if everything is a component would probably not work

thheller18:11:29

it still sounds like you are trying to solve the wrong problem to me

thheller18:11:58

reloading all namespace is going to be too slow. therefore experimenting with it is a dead end

thheller18:11:24

if the problem is just that the === on the component type fails then that is an easy fix

lilactown18:11:38

I can’t tell if that is the problem yet. The first problem is you need to call register with a type and ID for each component you want to refresh, each time you perform a refresh

thheller18:11:14

thats easy?

lilactown18:11:42

well I’ve been trying to think of how to refresh only the dependent components. if I refresh every component… it might be easier

lilactown18:11:03

if I could detect what components are in the dependency tree of a reloaded ns, without having to actually reload the ns they’re in, then that could improve the speed a lot

thheller18:11:23

you are approaching this incorrectly I think

thheller18:11:34

let me try what I have in mind

lilactown18:11:01

okay. I appreciate it

thheller18:11:45

yeah don't know. I can't get it to refresh anything. not even the root itself

thheller18:11:24

might check it out a bit later ...

lilactown18:11:01

? is my example not working? or were you trying something else

Nolan21:11:05

seems pretty inconsequential in terms of affecting functionality, but is this warning something im causing? only happens for browser target, cant figure it out

------ WARNING #4 -  -----------------------------------------------------------
 Resource: node_modules/buffer/index.js:2
 constant module$node_modules$buffer$index assigned a value more than once.
Original definition at externs.shadow.js:3
--------------------------------------------------------------------------------