File in invalid path will mark namespace for reload

Question

Having a cljc file at path {{public/js/out/foo/bar.cljc}} with ns form {{(ns foo.bar)}} will result in namespace {{foo.bar}} being reloaded.

This is problematic because ClojureScript compiler will copy all the input files to {{:output-dir}} for source-map use. Lately as more libraries have started using cljc, and this has started causing problems in cases where library is used on Clojure env. Cljs compilation will result in reload of the library code, which can redefine protocols etc. and break the Clojure env.

I think it would make sense for tools.namespace to ignore changes where file path and namespace don't match.
Another question and perhaps way to fix this, is to understand why dependency resolution doesn't work in this case: namespaces which depend on the protocols at a cljc file on output-dir aren't reloaded.

Answer 1

Comment made by: deraen

This is nearly the same to http://dev.clojure.org/jira/browse/TNS-24, but this case the inconsistently named ns will be reloade because there is a copy in the correct path.

Answer 2

Comment made by: deraen

Proposed patch with a test.

{{find-sources-in-dir}} is the easiest place to do the check, because we need the directory path.

• Possibly breaking for direct users of {{find-sources-in-dir}}?
• Now both {{find-sources-in-dir}} and {{file/files-and-deps}} need to read the ns form.

Answer 3

Comment made by: deraen

Fixed a typo on the patch.

Answer 4

Comment made by: severeoverfl0w

I've also experienced this bug. It's particularly bad when combined with bidi due to it's use of protocols and records. This patch resolved the issue for me perfectly.

Answer 5

Comment made by: stuart.sierra

The root cause of this problem is the common practice of putting compiled "web assets" such as compiled ClojureScript code on the Java classpath at {{resources/public}}.

Two easy solutions are available right now:

1. Configure ClojureScript to put compiled files in a directory not on the classpath
2. Use (link: http://clojure.github.io/tools.namespace/#clojure.tools.namespace.repl/set-refresh-dirs text: set-refresh-dirs) to omit those directories from the tools.namespace search.

Anything which changes the behavior of tools.namespace, especially in the lower-level APIs, has the potential to break other use cases, so must be considered carefully. For example, a code linter might use {{find-sources-in-dir}} to search for all Clojure source files, including those with invalid {{ns}} declarations.

Answer 6

Comment made by: deraen

1. Configure ClojureScript to put compiled files in a directory not on the classpath

Using classpath to serve files is the most common way, and is unlikely to change. Many tools, like Boot, encourage using classpath to serve files.

1. Use set-refresh-dirs to omit those directories from the tools.namespace search.

This is not possible on Boot. In Boot classpath is managed by Boot and one directory will contain files from several sources, e.g. Boot-cljs and {{source-paths}}.

Answer 7

Comment made by: severeoverfl0w

Would you accept this patch if do-refresh took an option in it's map of sane-paths-only? which it passes through it's calls down to find/find-sources-in-dir? Where find-sources-in-dir took an optional third argument specifying whether sane-paths-only??

It would be nice to refactor the final argument to find-sources-in-dir into an options map, but breaking that API is perhaps out of scope.

tl;dr would you accept this patch if sane-paths-only? only ran for the repl/refresh, and was passed down via arguments?

Answer 8

Comment made by: malcolmjuxt

I agree that you shouldn't compile artefacts to the classpath.

However, this is extremely widespread practise in the Clojure world, because of uberjars.

Personally, I don't like uberjars, but I seem to be in the minority! As long as people use uberjars in production, they will want to server from the classpath in development.

Hence I think that both the easy solutions Stuart proposes are unsatisfactory.

This is a really annoying bug for anyone who uses libraries that use cljc (e.g. bidi). It would be great if another solution can be proposed.

Answer 9

Comment made by: severeoverfl0w

1. Use set-refresh-dirs to omit those directories from the tools.namespace search.

Another issue I've noticed with this strategy is that it doesn't handle leiningen profiles with differing :source-paths in each. You have to alter the call to set-refresh-dirs each time you switch profile (and be confused when you get it wrong).

Answer 10

Comment made by: stuart.sierra

tools.namespace was never intended to be used in deployed applications, so I think it is safe to ignore the case of (link: https://dev.clojure.org/jira/browse/TNS-45?focusedCommentId=44822&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-44822 text: uberjars) for now.

If files in unexpected paths are skipped silently, it would be difficult to understand why a mis-named file is not being loaded. As a possible compromise, {{c.t.n.find}} could print a warning to * } when it encounters a file whose {{ns}} declaration does not match its path, and omit it from the search results.

That leaves the problem of spurious error messages. As noted in comments above, (link: https://dev.clojure.org/jira/browse/TNS-45?focusedCommentId=45080&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-45080 text: Leiningen profiles) and (link: https://dev.clojure.org/jira/browse/TNS-45?focusedCommentId=44149&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-44149 text: Boot) make it difficult to statically define the set of directories which *should be scanned for source files. If we could specify directories to *exclude from the search, would that be sufficient to work around the problem of ClojureScript copying {{.cljc}} files to a resources directory?

Answer 11

Comment made by: deraen

I think uberjars were mentioned in reference to why people serve files from classpath, not because people use c.t.n with deployed applications.

Would the excluded directories be defined as file paths or classpath prefixes?

In Boot the user doesn't control the directories which are included in classpath. Instead Boot creates a few temp directories which it controls and which are included in classpath. Files from several sources (project resource-paths, Cljs artifacts etc.) are all copied into same temp directory, so it is not possible to exclude Cljs artifacts based on that.

Defining classpath prefixes to ignore would probably work with Boot.

Answer 12

Comment made by: stuart.sierra

Excluding by classpath prefix might be possible, but tricky. At the moment, {{t.n.s.dir}} searches arbitrary filesystem directories, without knowing that those directories are on the classpath. Still, if exclusions were expressed as classpath-relative directory paths, they could be resolved to real filesystem paths.

Yet another possibility, which should have occurred to me earlier, is using file hashes, rather than timestamps as is done now, to determine when a file is "changed." This would prevent reloads of {{.cljc}} files just because the ClojureScript compiler has copied them. On the other hand, it would be a more significant change, and it would prevent the use of {{touch}} (or re-saving in an editor) to force a file to be reloaded.

Answer 13

Comment made by: stuart.sierra

New patch TNS-42-3.patch shifts the responsibility up a layer from c.t.n.find to c.t.n.dir. This avoids any breaking to c.t.n.find/find-sources-in-dir while achieving the desired behavior for c.t.n.repl/refresh.

This may be an adequate compromise, although I am still slightly concerned about mis-named files being silently ignored leading to confusion.

Answer 14

Comment made by: severeoverfl0w

Discarded files/namespaces could be added to the tracker, to mitigate confusion. Refresh could then warn about those files with prn as it does now.

Something would need to remove files from the discarded list if they ever became valid.

I'm not sure this should block TNS-45, but could be something for someone to pick up as follow-up.

Answer 15

Comment made by: stuart.sierra

New patch file TNS-45-4.patch prints a warning and adds the ignored directories to the tracker, following suggestions in (link: https://dev.clojure.org/jira/browse/TNS-45?focusedCommentId=47280&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-47280 text: comment) by Dominic Monroe.

This applies only to {{c.t.n.dir/scan-dirs}}, so other uses of the lower-level APIs in tools.namespace should not be affected.

On the first occurrence of a file not matching {{scan-dirs}} will print a warning will print a warning to * with the the declared {{ns}} name and the path to the file that did not match. After that, the entire directory will be ignored, and a notification will be printed on each invocation of {{scan-dirs}}. I hope this strikes a good balance between informative messages and not too many spurious warnings.

Note: previous patch file TNS-42-3.patch was mis-named but was always meant to apply to TNS-45.