Fork me on GitHub
#cljs-dev
<
2020-04-13
>
dnolen00:04:04

ha @mfikes fast initial prompt is actually pretty nice enabled on browser REPL

dnolen00:04:14

it was never that slow - but this makes it feel much faster

juhoteperi07:04:12

Oh, works now. I tested different command and left that in the configuration, with proper npx command I see the output file. I think it would be good idea to print out the webpack output?

juhoteperi08:04:54

Though for watch mode it definitely makes sense to not show the webpack stdout. Anyway, I can just run the command myself if I want to see the output, and probably makes more sense here anyway, as I might want to pass the bundle to webpack directly or to Karma.

dominicm08:04:11

RE: not showing the webpack output, I think I've made a mistake in my webpack config and it's taken me a good 15m or so to realize that's why cljs is silently not producing output.

juhoteperi08:04:13

If the command fails, the exception should contain the stdout contents.

dominicm08:04:34

Didn't fail, I think I didn't have webpack installed (I guess I wiped my node_modules). And it was prompting for input.

dominicm08:04:48

I guess when needing to prompt for input in non-interactive env it exits with 0?

juhoteperi08:04:49

Yeah. And I think it won't show stderr contents.

juhoteperi08:04:11

The implementation closes stdin, so it should exit?

dominicm08:04:31

ah, then in that case I guess npx read it as "N", and exited.

dominicm08:04:30

Something (possibly) useful is that you don't need a http://webpack.co

dominicm09:04:57

Interestingly, while the bundle output is smaller for react/react-dom, shadow still produces slightly smaller output. (1KiB difference).

dominicm09:04:47

For a simple prn, running through webpack seems to save some bytes:

prn/Cljs 1.10.520-fn-invoke-direct.js                           90KiB   21KiB   18KiB
prn/Cljs 1.10.520.js                                            92KiB   21KiB   18KiB
prn/Cljs 1.10.597-fn-invoke-direct.js                           93KiB   21KiB   18KiB
prn/Cljs 1.10.597.js                                            94KiB   21KiB   18KiB
prn/Cljs c1cf559a Bundle-fn-invoke-direct.js                    90KiB   20KiB   17KiB
prn/Cljs c1cf559a Bundle.js                                     91KiB   20KiB   17KiB
prn/Shadow-Cljs-2.8.94-fn-invoke-direct.js                      90KiB   20KiB   17KiB
prn/Shadow-Cljs-2.8.94.js                                       92KiB   20KiB   17KiB

dominicm09:04:58

(columns are raw, gzip, brotli)

juhoteperi09:04:17

For Reagent demo site, just using Cljsjs React is 20kB smaller

dominicm09:04:01

For very simple:

(ns io.dominic.cljs-sizes.main
  (:require
    [reagent.dom :as rd]))

(defn welcome
  [name]
  [:h1 (str "Hello, " name)])

(defonce _mount (rd/render [welcome "Fred"] (js/document.getElementById "app")))
I get slightly smaller bundle (by 8 bytes) for the webpack version.

juhoteperi09:04:43

I didn't have elide-asserts enabled for bundle builds, sec

juhoteperi09:04:58

466KB now, with Cljsjs 457KB

dominicm09:04:28

Does the demo site use more than react/react-dom?

juhoteperi09:04:31

ah no, that is not cljsjs version

dominicm09:04:47

I'm expecting bundle to win because it does not have so many duplicate dependencies as cljsjs creates.

juhoteperi09:04:20

reagent website in fact uses React with node module processing, so React code is going through Closure optimizations

juhoteperi09:04:17

node module processing 457KB, bundle 466KB, foreign-libs 470KB.

juhoteperi09:04:46

Makes sense now

dominicm09:04:10

ah, interesting. Probably not many sites that can do that!

juhoteperi09:04:45

No, React is probably one of the only libs that works and even it still requires the extern file.

juhoteperi09:04:37

Extern file also means that it isn't properly optimized, we should have separate extern file for this, with only the dynamic names that break when optimized, but not the public API which can be optimized in this case.

juhoteperi10:04:25

without the extern file, node module processing output goes down to 453KB (but it doesn't work)

juhoteperi09:04:07

Should ^:export work in advanced optimized bundle, so that the exported fn can be accessed from the JS side? Looks like goog.global is not the same as window and looks like it is not seen outside of Cljs module.

juhoteperi09:04:09

Maybe Cljs should set goog.global = window somewhere, goog.string.unescapeEntities also breaks as it doesn't find goog.global.document.

juhoteperi09:04:24

With few inferred extern additions, I now have Reagent tests passing with advanced optimized bundle.

dnolen10:04:43

@juhoteperi @dominicm re: :bundle-cmd output I really don't care that much here - it's just important to note it's a convenience

dnolen10:04:13

as noted you can easily run the bundle cmd separately and that might be common because you want to do more things

dnolen10:04:29

and the :bundle-cmd is intentionally simple and doesn't support parameterization

dnolen10:04:39

not interested in that at all

dominicm10:04:47

One problem is that I don't understand how bundle relates to development mode at all

dnolen10:04:00

I don't know what you mean?

dominicm10:04:15

Well, I have this "worry" that it'll be run more frequently than just once. So dev would be very inconvenient otherwise.

juhoteperi10:04:06

Bundle-cmd is run after every compile. Or I guess you could use JS tools to watch for file changes.

dominicm10:04:29

How often is a compile run?

dominicm10:04:36

e.g. on file change?

juhoteperi10:04:52

In watch mode, on file change.

dominicm10:04:59

Okay. One worry I have is webpack failing mid-development session. That would be a bit of face meeting wall to realize/debug.

dnolen10:04:36

that possible but I don't think it's much of a worry - you need the bundler when your ClojureScript requires include something from node_modules you haven't required before.

dnolen10:04:53

also nothing is stopping anyone from using watchman and setting this up however they like

dnolen10:04:06

:bundle-cmd is not important

dnolen10:04:53

@juhoteperi we could change ^:export here, goog.exportSymbol takes a third parameter - I'm wondering if that's good enough

juhoteperi10:04:55

Closure library code which presumes goog.global is the window object is also a problem, that wouldn't help with that

dnolen10:04:28

@juhoteperi ah yeah, ok we have the hook for this in core, only used in Node.js right now

dnolen11:04:25

if that works for you will close 3227

thheller11:04:59

can we please not do this?

thheller11:04:07

window does not exist in web workers

thheller11:04:22

react-native also doesn't have window

thheller11:04:55

there are plenty of things that might want to use bundle but don't have window

juhoteperi11:04:54

Maybe webpack has some way to handle this?

thheller11:04:06

the "problem" is that goog.global = this breaks because this is no longer window after webpack has processed it

thheller11:04:33

assigning goog.global in cljs.core is also too "late" given that it may have been used before loading cljs.core by closure code (eg. goog/base.js)

thheller11:04:13

in shadow-cljs I overwrite it directly in goog/base.js before emitting that file

juhoteperi11:04:19

Cljs should probably use similar logic to select the global object?

thheller11:04:55

thats not actually the correct code

thheller11:04:29

that is used for :npm-module which is a rather special :target in that it exposes each CLJS ns as a separate require("shadow-cljs/cljs.core") ns

thheller11:04:08

but if you assume webpack then I guess that would work regardless

thheller11:04:32

but I think the next version of webpack will remove the process polyfill so that would break then

juhoteperi11:04:33

@dnolen Export & unescapeEntities works now (but I had other problems with Karma)

juhoteperi11:04:52

maybe instead of checking for process.browser the global object could be just window || self || global ?

dnolen11:04:07

@juhoteperi I don't really know what you're trying to suggest

juhoteperi11:04:19

^ related to @thheller comments

dnolen11:04:39

I think automatically checking for those cases is a bad idea w/ limiting to target

juhoteperi11:04:34

Yeah, maybe it is best to leave goog.global intact by default? MAYBE option to set it to something IF webpack or other tools don't have better way handle this. It depends on where the bundle is used, what the global object should be.

dnolen11:04:56

Another option instead of mucking around with goog/base.js - we could just make a new compiler option to set this

dnolen11:04:11

independent of target, because I see that conflating is an issue

dnolen11:04:57

@thheller I've never personally seen cases where setting target at the end of core.cljs is a problem - do you have something specific? this after implementing in a bunch of environments, browser, Node, React Native etc.

thheller11:04:36

might be less of on issue today with the new goog.define logic

thheller11:04:56

but it used to be a problem that the goog.define in goog/base.js would then be defined in another scope

dnolen11:04:31

yeah it's not .a problem as far as I could tell

juhoteperi11:04:49

Webpack has way to set module this to window: https://webpack.js.org/loaders/imports-loader/

dnolen12:04:19

@juhoteperi right but this will be different in every bundler - I'm ok w/ having a way to control this because ^:export is critical

juhoteperi12:04:32

Yeah. Separate compiler option sounds ok to me.

dnolen12:04:57

no new compiler option, it's just a new goog-define

dnolen12:04:37

only supports string value: "window", "global", or "self"

dnolen12:04:48

@juhoteperi let me know if that works for you

dnolen15:04:54

feedback welcome!

dnolen15:04:19

a lot of the changes/fixes to master were based on a new React Native tool I've been working on for the past three weeks

dnolen15:04:01

if you're curious how to leverage cljs.main and the new build target in a custom build tool - this is worth checking out šŸ™‚

lilactown15:04:16

re: krell, very cool. it looks like it doesnā€™t support hot-loading yet; do you use a REPL workflow atm?

lilactown15:04:36

Iā€™m v interested in creating an easy hot reloading experience that takes advantage of the new react-refresh package. Itā€™s a little tricky tho.

lilactown16:04:09

react-refresh requires that when changing a file, all files in the dependency tree up to the root will be reloaded. This is to support e.g. changing a function or constant in a file, that gets required by another file, that then gets required and used in a component, will properly invalidate the component and trigger React to re-render that component. figwheel and shadow-cljs typically takes the simpler approach of only reloading the file changed and itā€™s direct ancestors because we assume everything is in the global scope and that we donā€™t do side-effects on reload (which react-refresh is essentially a side-effect on reload)

dnolen16:04:48

hot-loading is under development, but this ClojureScript so it's trivial

dnolen16:04:15

re: REPL - that's mostly what Krell does - REPL stuff - the bundler stuff is just reusing ClojureScript master

lilactown16:04:54

I feel like if I could figure out how to handle these react-refresh cases from a REPL, it would make everything else easier

lilactown16:04:39

the ā€œside-effect on defn ā€ pattern that react-refresh heavily encourages makes it more difficult

dnolen16:04:03

@lilactown I think the only thing is to not swim upstream

dnolen16:04:16

Krell auto-reconnects after a React refresh

dnolen16:04:27

so what you're talking about probably just works

lilactown16:04:48

hmm not sure yet

dnolen16:04:13

the point is you cannot preserve the REPL state - but depending on what you're doing who cares?

dnolen16:04:43

the point of auto-reconnect is that you can just follow all the usual React guidelines

dnolen16:04:55

no divergence

lilactown16:04:53

yeah, I want to preserve the behavior you get when enabling ā€œFast Refreshā€ in React Native

dnolen16:04:54

I'm not saying of course that react-refresh works - just saying that's definitely one of things I'm interested in doing different w/ Krell

dnolen16:04:10

that is if you want to use react-refresh Krell should get out of your damn way

dnolen16:04:45

disabling React refresh should be entirely an optional thing because you want to do a stateful REPL session

lilactown16:04:15

yep, mostly thinking out loud hoping to glean some good ideas šŸ™‚

lilactown16:04:42

I think thereā€™s two kinds of refresh in RN: a full reload of the app bundle on change, and the new ā€œFast Refreshā€ feature IIRC

dnolen16:04:43

my good idea - don't mess up React stuff and React workflow if users want it

dnolen16:04:50

no other good ideas

lilactown16:04:11

The ā€œFast Refreshā€ mode preserves state across reloads - even local component state - but requires you to generate signatures for each component that get invalidated on file change. this is done by a babel plugin that RN gets shipped with, which obviously wonā€™t work with CLJS

lilactown16:04:57

a react wrapper lib (like helix/reagent/etc.) can generate these signatures using a macro, but requires integration at the reload step to properly invalidate them like I was saying

lilactown16:04:17

I dunno, just thinking out loud. thanks for listening

dnolen16:04:02

oh because they process the JS

dnolen16:04:18

yeah, I dunno I'd have to look more closely at how it works to have any clearer thoughts

dnolen16:04:36

@lilactown but one question I have is how is react-refresh really any better than just developing a RN Reagent app over a state atom w/ Figwheel / shadow / soonish Krell?

lilactown16:04:19

the ability to reload while preserving local component state is huge

dnolen16:04:51

if you write all state into an atom same result no?

lilactown16:04:01

it completely changes the way that the DX encourages you to architect your app

dnolen16:04:16

In a web app this often not that fun - but for shallow view graphs in mobile I don't see the problem

lilactown16:04:48

if you take the stance that local state and global state are functionally and architecturally identical, I agree with you.

dnolen16:04:15

that's all I'm interested in though - if the end result is same performance, same experience

dnolen16:04:22

what do you need the distinction for?

lilactown16:04:37

IME the performance and experience is not the same

dnolen16:04:15

I haven't followed along but you also have Om inspired projects - the reconciler does what you're talking about

dnolen16:04:29

Om's idea is that you have global state - but the reconciler gives you precise updates

lilactown16:04:33

especially with concurrent mode unlocking more perf and experience knobs for devs, which works best with local component state

dnolen16:04:17

like I said I don't have a strong opinion and I personally think there are alternatives

dnolen16:04:10

reading over https://reactnative.dev/docs/fast-refresh it doesn't seem very simple and lots of caveats - so meh

orestis16:04:24

Havenā€™t dove down to fast-refresh for React Dev yet, but I can comment on the state bit - my goal is to take advantage of the new React Concurrent mode when it lands. So far, and until some new docs land, React wants to you let it handle state (there was months of churn to figure out how this could work with global state).

orestis16:04:08

This also impacts you more the more you rely on hooks, and the more you rely on JS components that use hooks. Fast-refresh promises to keep state from hooks and rerender, for free. Seeing that esp. for UI work local state makes some sense, preserving that makes sense.

orestis16:04:41

I donā€™t have much more insight :)

dnolen16:04:41

@orestis yeah I haven't followed the Concurrent mode stuff that closely - it does look interesting - but I don't know what you mean by "React wants you to let it handle state"

dnolen16:04:59

you write a functional component, you use a hook? what aren't you handling?

lilactown16:04:34

what he means is that, Concurrent mode works best when your state is coupled to an instance of a component.

lilactown16:04:19

construction/destruction relies on mount/unmount, updates to state mean the component re-renders, etc.

lilactown16:04:11

concurrent mode allows React to pause rendering a tree and resume rendering that same tree later. this creates problems if the state that was rendered in multiple parts of the tree has changed in between pausing and resuming

lilactown17:04:09

React can handle this in a transactional fashion if you manage state inside the component instance. it essentially puts state updates on a queue that gets computed on the render that it triggers. if you store state outside of a component instance, then React canā€™t handle those state updates for you - if you update an external state atom in a naive synchronous way then you can end up with parts of your tree that were rendered using an old atom state, and parts that were rendered using a newer atom state. if you store state once, and it lives in a component, then resuming the tree will either re-use the state updates already computed inside that render, or compute those state updates against the latest state. you avoid the sort of coherency problems that state external to the tree can introduce.

lilactown17:04:57

react is adding more tools to handle external state but the happy path is definitely storing it inside a component using useState / useReducer etc

dnolen17:04:11

yeah gonna have to look at the docs more - thanks for summary's though - helpful

šŸ‘ 4
orestis17:04:15

The most annoying thing is that this is very much in flux. This approach for examples invalidates all the major React global state managers (redux, relay, Apollo). Thereā€™s an RFC called useMutableSource IIRC that seems to handle this, and to my knowledge @mhuebert did a POC some time ago for ClojureScript. Iā€™ve kind of decided that Iā€™ll wait until an official release or more guidance is available.

orestis17:04:11

My only fear is that as with fast-refresh, the React devs will end up with just a Babel/webpack plug-in that we will have a hard time incorporating into our dev workflow.

dnolen17:04:37

yeah that was my immediate impression

dnolen17:04:42

not compatible w/ existing libs

dnolen17:04:18

in fact that's the part that I find kinda busted about it

orestis17:04:23

If I had time to focus on this Iā€™d definitely try to reach out to the React team, they said they wanted to release concurrent mode under an experimental release so that tooling can catch up. Sadly my day job leaves zero time for this kind of low level stuff.

dnolen17:04:28

why not just make a new library - don't call it React

orestis17:04:17

Heh, JS doesnā€™t work like that. React-router is in its 6th or 7th version.

lilactown17:04:51

they have lots of paths for migration

orestis17:04:02

But to be fair to them, they are trying the best and using the new Facebook as a guinea pig.

orestis17:04:58

The plan AFAICT is that the official release will be simultaneous with major libraries so the change for consumers will be mostly under the hood.

dnolen17:04:38

well this still seems a bit far out to me and upgrading legacy applications seems like a monumental task

dnolen17:04:49

so not much bearing on a RN REPL at this point šŸ˜›

lilactown17:04:30

yep šŸ˜› all of this CM talk is just motivation for why I desire good react-refresh integration

lilactown17:04:37

but it sounds like I can hopefully build it myself if need be

lilactown17:04:44

it works quite well with shadow-cljs right now too

lilactown17:04:06

I expect that libs and apps that use non-CM safe patterns will continue to be supported for a long time. there are a lot of different ā€œmodesā€ that allow devs to tune React to what works with their code

orestis17:04:26

Actually the new bundle stuff sounds like we can finally start releasing tools that depend on React and other npm stuff more easily.

lilactown17:04:35

the benefits to adopting CM are mainly around unlocking better UX

dnolen17:04:48

@orestis yes that's the point

orestis17:04:03

Super happy about it, thanks

mhuebert17:04:19

I totally wish React had put Concurrent into a new lib with different name. I am just glad that this useMutableSource stuff is finally arriving. Looks to me like it is going to work well.

Alex Miller (Clojure team)17:04:21

@dnolen was there a tools/repls page that got removed on the cljs site?

dnolen17:04:18

@alexmiller which one? I deleted a lot of stuff

Alex Miller (Clojure team)17:04:18

yeah, guess so - that was the top nav link for tools, so this broke a bunch of nav

dnolen17:04:42

yeah that's just way out of date don't want people looking at that anymore

dnolen17:04:53

that's before we switched to 1.X naming I think

dnolen17:04:25

@alexmiller you're not talking about internal links right?

dnolen17:04:36

I can clean that up - but nav stuff I don't think I can

Alex Miller (Clojure team)17:04:57

I'm talking about the top level nav - the Tools link went to that page

Alex Miller (Clojure team)17:04:10

I'm fixing it up now that I understand what happened

Alex Miller (Clojure team)17:04:16

I thought I broke it :)

dnolen17:04:58

ah k, I didn't understand nav worked that way - it shouldn't have been the top link at this point

Alex Miller (Clojure team)17:04:44

I can easily add redirects from old links to somewhere new too if you run into that, there's a list of those in the deployment

Alex Miller (Clojure team)17:04:46

should be fixed up now - there's now a tools/tools page which is in the clojurescript-site that mostly matches the left nav

orestis18:04:09

@dnolen not sure if you are monitoring ClojureVerse, some comment left there on Node versions: https://clojureverse.org/t/test-drive-the-beta-clojurescript-js-bundler-integration-support/5779

dnolen18:04:28

@orestis chimed in, not sure what's going on there seems similar to what @juhoteperi but I think missed the root cause

dnolen18:04:39

I did the tutorial with Node 10.16.0 myself

orestis18:04:03

Cool I didnā€™t know if you had an account :)

juhoteperi18:04:27

Related to JS modules, I've been already thinking about adding notice to Cljsjs page and repository strongly suggesting using other ways to consume JS libraries. Now is even better time for that.

šŸ‘ 4
dnolen18:04:17

@juhoteperi yes, but let's wait for the release

dnolen18:04:43

then it doesn't matter what tool people use they can follow that guideline easily

juhoteperi19:04:43

I close this one old module processing issue, couldn't reproduce the problem now: https://clojure.atlassian.net/projects/CLJS/issues/CLJS-2836

dnolen20:04:03

šŸ‘:skin-tone-4:

juhoteperi20:04:00

I think https://clojure.atlassian.net/browse/CLJS-1543 this is now done (goog.module support)?