Cljs system integration

[featheredtoast]

Here's a working proof of concept with system integration for cljs for you to review. I've been favoring this setup in a personal project and the patch here is a simple port back to chestnut.

Go ahead and see how you like this. In general, I'd love to hear what you're envisioning for component integration on the cljs side, and I hope this patch is a good starting point for discussion.

• This is currently dependent on my cljs fork of reloaded.repl.
• I've modified the repl :init to run (go) for dev ease. This can go if it's unwanted.
• The sample component and system is copy-pasted to each of the cljs templates. Currently this system prints out messages on start/stop. I'm sure there's a better way to do this bit.
• I've added a dev/cljs/user.cljs for dev builds that refers to the reloaded repl cljs' go, start, stop, and reset functions for easier control in an attempt to mirror the same kind of control as clojure's user namespace.
• A figwheel hook resets the system on code reload.
• [x] It's currently including the recently submitted system patch fix so it's in a working state. I'll rebase once that patch is merged.

[plexus]

Hey @featheredtoast, thanks a lot for the pull request. Sorry it's taken me a while again, but I've been pretty busy, and I wanted to make sure I had had a good look at this before commenting.

I had a look at your commits, and generated a project to get a feel for it. I also had a look at the code of reloaded-repl-cljs.

I'm coming around to having Component on the CLJS side as well. It's a pretty bold move, Component seems to be used a lot less in CLJS than in plain Clojure, but I think there is merit to it. It will also give us a good structure to plug into when adding extra features/flags like +sente.

I'm not really sure yet if there should be a +component-cljs flag (which is implied when doing e.g. +sente), or if it should be always there. From a user point of view I think the flag would be good to have. In Chestnut we try to avoid superfluous boilerplate, and for a lot of people this will be exactly that, boilerplate that doesn't add anything.

From a Chestnut dev POV I would avoid the flag though, because it has a pretty big impact on how things are organized, and it will become very easy to break things either with or without the flag. So... I'm leaning to having Component always there.

Regarding the specifics of the code, what do you think of splitting things in multiple files, and using a similar pattern as used in CLJ? Have a look at this example project.

https://github.com/plexus/sesame/commit/47b110cad34dc59eaee03f7eb9a85a5cc44a37cb

It wraps the reagent/render call in a component. Since this is part of booting the app I think that makes sense, and it means we don't have to have an ExampleComponent there that doesn't do anything.

I rolled the reloaded-repl functionality into the code itself. Since in CLJS there is no tools.namespace and no explicit reloading (we count on Figwheel instead), I think calling it reloaded-repl is a misnomer. And without the reloading it's so little code maybe it doesn't need to be a separate lib. (Although it could be, but then maybe something with a better name).

One thing your code didn't address is how the app will boot in production (uberjar). Here I've used the pattern that re-frame uses of having an inline <script> that starts the system.

[featheredtoast]

No worries about rushing - take your time, I'd rather like to get this right.

I really like your iteration a lot - Definitely makes sense to separate the cljs code for the component, and I also do think that your solution of having a separate script to start the system inline in the HTML is cleaner. (While having the go call here allows the uberjar to boot fine, I do think your delcaration in a script tag is more sensible to reason about.)

I agree that declaring and bootstrapping the app via component makes a lot of sense. I also don't really have a preference either way regarding the reset functionality as a lib or built into chestnut. I am coming around to having it just be a part of the template as a library doesn't exactly "get" you very much in terms of abstraction. I'll go that route for the next iteration here. (It also saves us from having to name something.)

My one piece of criticism about the changes is in the cljs.user namespace. It seems like something in google's closure compiler is gutting the ability to call the functions in the repl. From what I've tested, simply importing functions via "use/only" or "require/refer" is not enough to expose the functions to the repl without some sort of use in the user namespace itself. On interacting with it from the repl, I get the following:

cljs.user> (go)
----  Compiler Warning on   <cljs form>   line:1  column:2  ----

  Use of undeclared Var cljs.user/go

  1  (go)
      ^--- 

----  Compiler Warning  ----
#object[TypeError TypeError: Cannot read property 'call' of undefined]
nil
cljs.user> cljs.user
#js {}
cljs.user> 

That's the reason for the explicit def calls in the cljs.user namespace, even though they look like sin. (rebuild your cljs, and load a new browser window, then you should see this.)

I'll update the PR with the feedback and get back to you.

[plexus]

Makes sense about the def calls. I've also started doing this more because I use clj-refactor a lot, and it will clean unused vars out of the namespace declaration, so this is a nice workaround to make sure they "stick" also in Clojure.

[featheredtoast]

Alright, rebased the original on current master, and added a second patch for mounting via render methods.

• component starts with go function called in script tag
• pulled in the go/stop/reset functions
• each rendering method (om, reagent, etc) exposes 'render' function, rather than rendering directly.
• generic renderer component responsible for rendering/mounting app (this will need a better name)

A few extra notes: As I was working on this, it made the most sense to expose a single render method on each render type, rather than create a component per type. Also, it didn't make sense to me to unmount the app if you stop the system, so I left that part of the implementation out. Usually when I'm debugging, I'm stop/start/reset the system and want to see how it interacts without changing the way things are currently rendered. Let me know what you think. Happy to rebase + squash once reviewed!

[featheredtoast]

I figure you've been busy; have you had a chance to further reason about cljs system integration?

[plexus]

This looks good. I'm merging it and will have a play around with it, we can further improve upon it on master.

Part of me wants to make Component on the frontend side optional (feature flag), as it's much less common than on the backend. But that would mean two pretty different implementations that we need to keep up to date, so let's just go with component as the default. I think the way it looks now we can keep the boilerplate to a minimum, and have it in separate files so people who don't care for component on the frontend can mostly just ignore it.

Thanks again for the patch!

[plexus]

@featheredtoast I renamed RenderComponent to UIComponent, and cleaned up some whitespace. For the rest I think we're good!