tools.namespace

File in invalid path will mark namespace for reload

Details

  • Type: Enhancement Enhancement
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None

Description

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.

Activity

Hide
Juho Teperi added a comment -

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.

Show
Juho Teperi added a comment - 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.
Hide
Juho Teperi added a comment -

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.
Show
Juho Teperi added a comment - 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.
Hide
Juho Teperi added a comment -

Fixed a typo on the patch.

Show
Juho Teperi added a comment - Fixed a typo on the patch.
Hide
Dominic Monroe added a comment -

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.

Show
Dominic Monroe added a comment - 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.
Hide
Stuart Sierra added a comment -

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 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.

Show
Stuart Sierra added a comment - 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 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.
Hide
Juho Teperi added a comment - - edited

> 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.

> 2. 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.

Show
Juho Teperi added a comment - - edited > 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. > 2. 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.
Hide
Dominic Monroe added a comment -

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?

Show
Dominic Monroe added a comment - 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?
Hide
Malcolm Sparks added a comment -

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.

Show
Malcolm Sparks added a comment - 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.
Hide
Dominic Monroe added a comment -

> 2. 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).

Show
Dominic Monroe added a comment - > 2. 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).
Hide
Stuart Sierra added a comment -

tools.namespace was never intended to be used in deployed applications, so I think it is safe to ignore the case of 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 *err* 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, Leiningen profiles and 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?

Show
Stuart Sierra added a comment - tools.namespace was never intended to be used in deployed applications, so I think it is safe to ignore the case of 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 *err* 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, Leiningen profiles and 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?
Hide
Juho Teperi added a comment -

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.

Show
Juho Teperi added a comment - 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.
Hide
Stuart Sierra added a comment -

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.

Show
Stuart Sierra added a comment - 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.

People

Vote (0)
Watch (3)

Dates

  • Created:
    Updated: