Add output-dir option #65

ghost · 2016-03-10T00:04:54Z

The old asset-path option had the opposite behavior of the same option
in the clojurescript compiler. It was performing a role similar to
clojurescript's output-dir, and there was no way to get the expected
asset-path behaviour.

See https://github.com/clojure/clojurescript/wiki/Compiler-Options

This commit renames the old asset-path to output-dir, and adds a true
asset-path option.

Deraen · 2016-03-10T06:18:54Z

test/adzerk/boot_reload/server_test.clj

+           (web-path {:protocol "http:"
+                      :output-dir "resources/public/js/saapas.out"
+                      :asset-path "js/saapas.out"}
+                     "resources/public/js/saapas.out/saapas/core.js")))))


Is this a real use case?

Writing files to resources directory doesn't make much sense, and even if it was the target directory it wouldn't make much sense.

It's the same use case whether or not the "resources/" is there.

Deraen · 2016-03-10T06:30:49Z

Thanks. This will need some thinking as I would prefer not to break current behavior and I don't yet completely understand the use case for this.

Can you describe the use case? I guess example project would be really useful.

This takes us sadly closer to declarative Leiningen configuration where users have to configure paths to multiple plugins to get them work closer. Preferred solution in Boot would be to infer the configuration from files in fileset created by user (.cljs.edn), from files created by other tasks, or from metadata.

smw · 2016-03-10T06:48:57Z

From reading the linked pull request, this is more about where the directory/file is mounted as seen by the browser than where you want to output on the local file system?

ghost · 2016-03-10T11:17:14Z

I see your point about having a redundant configuration, and I do agree it is not a good thing, but it is better than a non-working configuration. In my use I have found that:

(serve :dir "resources/public")
(reload :ids #{"project"})
(cljs :compiler-options {:asset-path "js/project.out"})
(target :dir #{"resources/public/js"})

does not reload. (Don't get hung up on "resources/", the part that makes this not work is "js".)

And similarly:

(reload :ids #{"project"} :asset-path "js/project.out")
(cljs :compiler-options {:asset-path "js/project.out"})

just makes things worse. The two textually adjacent uses of :asset-path do opposite things and do not play well together.

The ultimate motivation for this is development with nginx. I'm writing a clojurescript app that needs to access a backend not in the same project, so to make everything appear to be at the same domain in development, I use nginx to reverse-proxy the backend and serve static files out of "resources/public". (I'm also using HTML5 history which doesn't work with the baked-in webserver as best I can tell, so there are multiple reasons to bring in nginx here.)

With this setup, loading the js works the first time, but reloading doesn't work, because the paths that get sent over the websocket are foobarred and can't be fixed by any existing option.

Yes, it is a backwards-incompatible change and would require a corresponding version bump if accepted, but the current situation:

Runs counter to the existing clojurescript ecosystem (i.e., clojurescript itself).
Complects multiple concepts that really should be kept separate.
Runs counter to user expectations, as evidenced by your issue list.
Makes transitioning to boot unnecessarily difficult for former leiningen users.
Does not work for everyone.

All-in-all, it is just better to keep separate concepts (asset-path, output-dir) separate.

If it is possible to avoid the redundant configuration by inferring the necessary config from elsewhere, I'd be happy to rewrite this patch to do so, but I would appreciate some kind of pointer in the right direction.

pandeiro · 2016-03-10T13:12:16Z

@esessoms First, not directly related to this PR, but boot-http (as of 0.7.3) supports a :not-found handler that might solve your HTML5 history requirement in development. You can also use middleware to create this proxy within Ring/Clojure, see for example this PR, which may obviate the need for this complex setup involving nginx serving ad-hoc paths.

ghost · 2016-03-10T14:40:54Z

Thanks @pandeiro I will give :not-found a try.

ghost · 2016-03-10T15:19:23Z

Example project, as requested:

https://github.com/esessoms/minfail

Deraen · 2016-03-10T16:11:54Z

@esessoms Not sure if it helps at all here, but you should not use target task and serve :dir in development. Instead it is good practice to serve files from classpath using serve :resource-root. Or ring wrap-resource. (Very short explanation: target directory is shared state and shared state is bad. I guess me or someone should finally write proper explanation somewhere, and how Boot solves this using tempdirs, fileset etc.)

Objective with boot-cljs + reload is to work automatically when user is adhering to some convetions, like serving files from classpath. Some option can be provided for other use cases, but I consider that secondary objective. HTML5 is such special case which I guess will require some configuration.

Deraen · 2016-03-10T16:13:21Z

Also, as quick fix, I'm open to adding new options, but it would be much preferred if they didn't break existing set ups (i.e. rename existing option).

nashbridges · 2016-03-10T16:19:01Z

@Deraen, not sure if you noticed my issue #64. Please, take into account that not all boot users are also clojure users. In the linked ticket I propose unified :asset-path-fn (and eventually deprecating :asset-path), however, I'm not brave enough to commit to clojure projects yet.

Deraen · 2016-03-10T16:22:35Z

Yes, I keep that in mind.

One solution I've been talking with @micha (for over a year now, probably) is having some kind of .web.edn file (similar to `.cljs.edn) which would define necessary attributes (asset-path at least) for all the Boot tasks to use. Then the options would only be defined once and both boot-cljs, boot-reload (and others) would read the same file.

Possible asset-path could even be infered from the relative path of file inside fielset, like boot-cljs does with .cljs.edn and cljs output-path.

The old asset-path option had the opposite behavior of the option of the same name in the clojurescript compiler. It was performing a role similar to clojurescript's output-dir, and there was no way to get the expected asset-path behaviour. See https://github.com/clojure/clojurescript/wiki/Compiler-Options This commit adds a new option cljs-asset-path that corresponds to the clojurescript asset-path.

ghost · 2016-03-10T16:55:05Z

@Deraen I do not doubt that your advice on proper usage is correct. I'll just note that it contradicts the boot faq, the boot-cljs example project, and the recommended tutorial. That is, all the documentation is out of date in this respect.

https://github.com/boot-clj/boot/wiki/FAQ
https://github.com/adzerk-oss/boot-cljs-example/blob/master/build.boot
https://github.com/magomimmo/modern-cljs/blob/master/doc/second-edition/tutorial-03.md#enter-deftask

But I have restored the old asset-path in my latest push, and added a new option cljs-asset-path.

Deraen · 2016-03-10T22:22:43Z

@esessoms You are correct, I've been slacking with documentation, I guess we should really support both cases, and with proper implementation it shouldn't really matter if one serves files from classpath or filesystem.

Saapas and Tenzing serve files from classpath and they listed in the same list as modern-cljs so not all documentation/examples are "wrong" :)

tiye · 2016-03-24T08:24:04Z

How is this issue going now?

Deraen · 2016-03-24T08:34:47Z

The changes look good now. I will still need to find some time to test this with few projects before a release.

Deraen · 2016-03-24T09:09:21Z

Well, I found some time quite soon :) This is now deployed as 0.4.6.

tiye · 2016-03-24T09:18:01Z

👍

martinklepsch · 2016-06-23T14:53:52Z

src/adzerk/boot_reload/server.clj

+     (if (= "file:" protocol)
+       (.getCanonicalPath (io/file target-path rel-path))
+       (str
+        (if cljs-asset-path (str cljs-asset-path "/") "")


@Deraen @esessoms I think this introduced a bug: paths are no longer starting with / breaking reloading from pushState locations. Should this line perhaps be:

(str cljs-asset-path "/" ...)

So that the slash is always present?

ghost mentioned this pull request Mar 10, 2016

Prepend :asset-path #64

Closed

Deraen reviewed Mar 10, 2016
View reviewed changes

Deraen merged commit a96d917 into adzerk-oss:master Mar 24, 2016

martinklepsch reviewed Jun 23, 2016
View reviewed changes

martinklepsch mentioned this pull request Jun 23, 2016

make sure slash is prepended if no cljs-asset-host #83

Merged

js-choi mentioned this pull request Nov 16, 2016

Rename :asset-path #99

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add output-dir option #65

Add output-dir option #65

ghost commented Mar 10, 2016

Deraen Mar 10, 2016

ghost Mar 10, 2016

Deraen commented Mar 10, 2016

smw commented Mar 10, 2016

ghost commented Mar 10, 2016 •

edited by martinklepsch

Loading

pandeiro commented Mar 10, 2016

ghost commented Mar 10, 2016

ghost commented Mar 10, 2016

Deraen commented Mar 10, 2016

Deraen commented Mar 10, 2016

nashbridges commented Mar 10, 2016

Deraen commented Mar 10, 2016

ghost commented Mar 10, 2016

Deraen commented Mar 10, 2016

tiye commented Mar 24, 2016

Deraen commented Mar 24, 2016

Deraen commented Mar 24, 2016

tiye commented Mar 24, 2016

martinklepsch Jun 23, 2016

Add output-dir option #65

Add output-dir option #65

Conversation

ghost commented Mar 10, 2016

Deraen Mar 10, 2016

Choose a reason for hiding this comment

ghost Mar 10, 2016

Choose a reason for hiding this comment

Deraen commented Mar 10, 2016

smw commented Mar 10, 2016

ghost commented Mar 10, 2016 • edited by martinklepsch Loading

pandeiro commented Mar 10, 2016

ghost commented Mar 10, 2016

ghost commented Mar 10, 2016

Deraen commented Mar 10, 2016

Deraen commented Mar 10, 2016

nashbridges commented Mar 10, 2016

Deraen commented Mar 10, 2016

ghost commented Mar 10, 2016

Deraen commented Mar 10, 2016

tiye commented Mar 24, 2016

Deraen commented Mar 24, 2016

Deraen commented Mar 24, 2016

tiye commented Mar 24, 2016

martinklepsch Jun 23, 2016

Choose a reason for hiding this comment

ghost commented Mar 10, 2016 •

edited by martinklepsch

Loading