-
Notifications
You must be signed in to change notification settings - Fork 99
Conversation
9338ebb
to
c8259a9
Compare
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 I'm not really sure yet if there should be a 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. It wraps the I rolled the reloaded-repl functionality into the code itself. Since in CLJS there is no 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 |
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:
That's the reason for the explicit I'll update the PR with the feedback and get back to you. |
Makes sense about the |
c8259a9
to
2657069
Compare
Alright, rebased the original on current master, and added a second patch for mounting via render methods.
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! |
- component starts with go function called in script tag - inline go/stop/reset functions - each rendering method (om, reagent, etc) exposes 'render' function - generic renderer component responsible for rendering/mounting app (might need a better name)
c1adb29
to
599d78b
Compare
I figure you've been busy; have you had a chance to further reason about cljs system integration? |
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! |
@featheredtoast I renamed RenderComponent to UIComponent, and cleaned up some whitespace. For the rest I think we're good! |
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.