Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for lein checkouts #517

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add support for lein checkouts #517

wants to merge 4 commits into from

Conversation

danielcompton
Copy link
Contributor

@danielcompton danielcompton commented Feb 2, 2017 •
edited
Loading

  • Add checkouts source directories to the classpath
  • Add checkouts source directories to :source-paths

This isn't necessarily ready for merge, I'm opening it to get a discussion about the way I've done this. I originally tried to get the checkouts information and pass it through into figwheel-sidecar.repl-api for it to add it to the source paths, but I think this way is cleaner and keeps sidecar agnostic to build tools.

It might be better to add the checkouts source paths in l.figwheel.cljs-builds or maybe in figwheel-main and build-once? I'm not quite sure yet.

Todo:

  • Docs
  • Do we need to support the old cljsbuild format when doing checkouts?
  • Add tests
  • Fix existing tests
  • Examples
  • cljsbuild seems to not add all of the source paths to be built, just the ones needed for development (i.e. not test). Not quite sure how they do that. @mneise do you know?
  • Test performance impact

Fixes #9

bhauman
bhauman reviewed Feb 2, 2017
View reviewed changes
plugin/src/leiningen/figwheel.clj Outdated
(source-paths-for-classpath
(normalize-data project build-ids))
(normalize-data project build-ids)
project)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you pass in the project created on line 437 to normalize data you won't have to change source-paths-for-classpath.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or alternatively you can just distinct-concat them onto the classpaths on line 438

bhauman
bhauman reviewed Feb 2, 2017
View reviewed changes
plugin/src/leiningen/figwheel.clj Outdated
(map (fn [build]
(update build :source-paths concat checkout-paths))
builds)))))

Copy link
Owner

@bhauman bhauman Feb 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add checkouts needs to consider [:figwheel :builds] as well.

@bhauman
Copy link
Owner

bhauman commented Feb 2, 2017 •
edited
Loading

The approach of doing this as early as possible is the best approach. If you append the checkouts to :source-paths they can automatically be added to the classpath.

This behavior should not happen if :watch-paths or :compile-paths is set.

You should take a moment to look at the new :watch-paths and :compile-paths, :source-paths functionality. These give the programmer a lot more control over the behavior of the system.

@pkpkpk
Copy link

pkpkpk commented Feb 2, 2017 via email

@bhauman
Copy link
Owner

bhauman commented Feb 2, 2017 via email

@pkpkpk
Copy link

pkpkpk commented Feb 3, 2017 via email

@mneise
Copy link

mneise commented Feb 4, 2017

@danielcompton Would you mind clarifying your question regarding cljsbuild, in particular "cljsbuild seems to not add all of the source paths to be built”? Not adding all of the source paths to where? Or do you mean it's not passing all of the source paths to the cljs build api? Thank you 🙂

@danielcompton
Copy link
Contributor Author

@mneise the way my PR is currently, it will add all of the source paths of all of the cljsbuild configs in a checkout project to the figwheel build.

Looking at the way cljsbuild behaves, it seems like it doesn't add all of the configs, only some, but I'm not sure how it does this. I could be misunderstanding things though.

@danielcompton
Copy link
Contributor Author

I'm curious: is there a perf cost to automatically watching checkouts? I'm pretty liberal with checkouts but i don't necessarily add them to :source-paths because it seems to slow things down

What is the use case for not watching and compiling checkouts but having them setup?

One of the things I'm trying to avoid with this PR is that when we have different team members working on the same project, we each might have different checkouts (or none at all), so it would be great if we didn't need to modify the config and keep that config change out of our commits that we publish back.

I'll do some testing to verify what the performance impact is of watching checkouts. Are you talking about the performance impact of watching more paths or of recompiling them? In both cases, I'm still not sure what the value of having knobs to turn would be, i.e. if there is a checkouts directory in place, why wouldn't you want projects in it to be watched and compiled?

@bhauman
Copy link
Owner

bhauman commented Feb 7, 2017

Daniel,

I just want to make sure that you know that I'm not sold on this functionality yet.

@pkpkpk
Copy link

pkpkpk commented Feb 8, 2017 •
edited
Loading

if there is a checkouts directory in place, why wouldn't you want projects in it to be watched and compiled?

Simply to get bug fixes/features without the overhead of watching a couple dozen files unnecessarily. In my experience, watching a ton of files drags on recompilation times and it breaks the fw spell. Maybe things have gotten better recently idk, but given variable OS watching apis, I think a user should be able to opt out

@stumitchell
Copy link

I'm quite keen on this functionality bacause leiningen automatically picks up checkout directories without changing a project file.
So it seems to me the expected functionality for figwheel should be to watch them as well.
It took me some time to work out what was happening when I found out this was not the case.

@bhauman
Copy link
Owner

bhauman commented Jul 25, 2017

@danielcompton if we make this a configurable option that defaults to off I'm open to moving this forward

@danielcompton
Copy link
Contributor Author

Ok great, I'll take another look at it and address the issues you raised. Not sure when I'll get the chance though, might be in a few weeks time.

@deg deg mentioned this pull request Oct 16, 2017
Closed
@deg
Copy link

deg commented Oct 16, 2017

What is the state of this PR?

I'd love to see it happen. But, if it remains controversial, perhaps a smaller change would be to modify figwheel-sidecar.components.cljs-autobuild/cljs-build to check for missing directories and just warn instead of err.

I'd obviously prefer no-warning builds in both production and development; both with and without a checkouts directory. The status quo gives me workable production builds with just a warning about the missing checkouts dir. But, when someone else tries running my projects under Figwheel, without checkouts, the hit a hard stop of a crash:

java.io.FileNotFoundException: checkouts/sodium/src (No such file or directory)
 at ...
    cljs.build.api$build.invoke (api.clj:189)
    figwheel_sidecar.components.cljs_autobuild$cljs_build.invokeStatic (cljs_autobuild.clj:28)
    figwheel_sidecar.components.cljs_autobuild$cljs_build.invoke (cljs_autobuild.clj:27)
    figwheel_sidecar.build_middleware.injection$hook$fn__29539.invoke (injection.clj:197)
    figwheel_sidecar.components.cljs_autobuild$notify_command_hook$fn__29927.invoke (cljs_autobuild.clj:68)
    ...

@danielcompton
Copy link
Contributor Author

@deg I hope to take another look at this PR again soonish to get it over the line, but can't give a solid date. Happy for you (or anyone else) to take my commits and open a new PR to finish this off.

@danielcompton danielcompton force-pushed the checkouts branch 5 times, most recently from 0c7e9c6 to 5c6ceda Compare December 17, 2017 22:48
@danielcompton
Add support for lein checkouts
013ba71 
- Add checkouts source directories to the classpath
- Add checkouts source directories to :source-paths

Fixes bhauman#9
@danielcompton
Copy link
Contributor Author

danielcompton commented Dec 17, 2017 •
edited
Loading

I've taken another crack at this and completely reworked it to be easier to test, understand, and modify. It's ready for another review.

This behavior should not happen if :watch-paths or :compile-paths is set.

I've checked this. If :watch-paths is set it doesn't override its settings, and if :compile-paths is set it doesn't override its settings. This behaviour felt natural to me.

I'm curious: is there a perf cost to automatically watching checkouts? I'm pretty liberal with checkouts but i don't necessarily add them to :source-paths because it seems to slow things down
... I think a user should be able to opt out

I added a large internal library that we have and couldn't notice any impact at all of watching the extra files. The recompile started as fast as I could save the files. If you want more fine control over the behaviour you can use :watch-paths and :compile-paths to tweak it to be exactly what you want.

I have been thinking about
:source-paths ["src" :checkouts]

I don't really like this approach, because it is no longer automatic, and now relies on the user adding the symlinked project, and updating the project. The beauty of checkouts is that it is configured automatically and "just works". This is also the behaviour that Leiningen and lein-cljsbuild have, so it would be surprising to have to add this config for Figwheel, and would probably also conflict with using the same config for cljsbuild.

@bhauman
Copy link
Owner

bhauman commented Dec 18, 2017

OK, this looks good, I would like an explicit opt-in option.

It is very plausible that there are a number of projects out there that have a lein checkouts directory that only have .clj files in them. With this change, these files will now be watched and hot loaded by figwheel by default. People will be surprised by this.

The main problem is that Figwheel is very different from cljsbuild as people rarely have a clojure REPL jacked into their cljsbuild process. People who work with Figwheel often have their regular http server code running in the same process as Figwheel which can make spontaneous reloading of a clj file that you are working on quite painful.

This is a big difference, given that currently Figwheel by default only watches files in the :source-paths of the configured ClojureScript build and it doesn't watch the files specified at top level lein project.clj :source-path. However this patch takes the :source-paths of the whole checkout project which may or may not include the ClojureScript source, and or may only include clojure sources that have no bering on ClojureScript building.

In terms of not breaking things for people who are currently using figwheel and as a method of trying this patch out I'd like to make it opt-in with a :honor-lein-checkouts boolean flag.

In response to not noticing lag:
FSevents are fast but not all projects use FSEvents, Hawk has an option for systems that don't behave well with FSEvents (docker and perhaps some windows situations) and it will iterate through all the watched files every second.

danielcompton
danielcompton commented Jan 10, 2018
View reviewed changes
plugin/src/leiningen/figwheel.clj Outdated
(source-paths-for-classpath
(normalize-data project build-ids))
(vec build-ids))))
(let [checkout-sources (checkout-source-paths project)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These could be added on https://github.com/bhauman/lein-figwheel/pull/517/files#diff-a2d728de692e56087ac2612b95a8bc4aL513 in the top level function, but it felt more appropriate to put them here. Happy to move them though, if you prefer.

@danielcompton
Copy link
Contributor Author

OK, this looks good, I would like an explicit opt-in option.

Great, I'll add this.

@bhauman
Copy link
Owner

bhauman commented Jan 11, 2018 via email

@atroche
Copy link

atroche commented Apr 28, 2018

Hey @danielcompton, some of your tests failed for me, because the relativized paths were coming through as e.g. “../utils-lib/src” rather than “checkouts/utils-lib/src”. I changed the tests and did a tiny refactor (just pulling a couple of things into let expressions) in this commit: atroche@3612f23

Do you think this could be a difference between our environments? I'm using OS X 10.12 and Java 1.8.

@atroche
Copy link

atroche commented Apr 28, 2018

And here's my attempt at adding the opt-in option for it:
atroche@56834e7

@atroche atroche mentioned this pull request Apr 28, 2018
Merged
@danielcompton
Copy link
Contributor Author

danielcompton commented May 1, 2018 •
edited
Loading

Will try and take a look at this again soon. I'd love to get it over the line, but I've been pretty busy and haven't quite had the brain space for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bhauman bhauman bhauman left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@danielcompton @bhauman @pkpkpk @mneise @stumitchell @deg @atroche