Remove options argument from link-to-asset

[RyanMcG]

Was part of #3. Also controversial. But generally less so considering validation will stop people from misusing precompile and the normal use case does not require options being passed into link-to-asset.

Update: Not making options into precompile optional and removing options from link-to-asset. This requires a new middleware which wraps each request in a with-options. This is necessary because many ring servers use many threads and dynamic vars are thread local.

[pbiggar]

Let's leave this one too, til the others get sorted out.

[RyanMcG]

We can possibly break this down some more.

1. Do you agree that options being passed into link-to-assets should be optional?
2. Do you agree that options being passed into precompile should be optional?

[RyanMcG]

Would it be helpful if I split this into two PRs? One for link-to-asset and one for precompile?

[pbiggar]

Having thought about this some more, I still really dont like optional. Make it required, or make it not available (requiring people to use with-options).

[RyanMcG]

Then lets remove it from link-to-asset and not precompile. When does it serve a use in link-to-asset?

[RyanMcG]

Final try here. Since dynamic vars are thread local, each handler must have a with-settings in it. Previous implementations did not work as expected.

I'm changing the title and description to match the fact that we are making link-to-asset not take options at all.

[boymaas]

Yes, I like this changeset. Just came to the same conclusion as Ryan. Took me almost an hour to figure out why my settings where not present by default in the link-to-asset function. Was just starting to write this change myself. From my point of view, I would like to have this merged.

[pbiggar]

@boymaas did you not have an error from calling with the wrong number of args?

[RyanMcG]

@pbiggar did you? I ran the test suite successfully and tried to find all invocations of link-to-asset and update them.

[pbiggar]

@RyanMcG I mean in @boymaas's case where he spent an hour on a wild goose chase.

[RyanMcG]

@pbiggar Ah, I see. I just assumed he passed in nil or {} like I'd been doing. Besides, an hour to figure out why with-options wasn't working around ring handlers doesn't seem that weird to me. Thread local binding isn't exactly something one usually have to think about. It probably took me more than an hour.

[pbiggar]

That's the thing, why would you put nil in for a required arg and expect it to work!

[RyanMcG]

why would you put nil in for a required arg and expect it to work!

Was that a question? I believe I've already answered it several times. One of the answers is because sometimes no options (e.g. nil) is valid and the other is because you shouldn't have to.

Here is the full story.

At first glance I didn't. However, having already passed in the same options to stefon when invoking asset-pipeline I wanted to DRY it up so I read some code.

Inspecting the code shows us that asset-pipeline wraps handlers in with-options. Doing some naive testing led me to believe that handlers already had access to those options. Passing in nil or {} to link-to-asset results in the new *settings* of (merge *settings* nil) (i.e. there is no change). Unfortunately, because of the multithreaded nature of most ring servers this doesn't usually work.

This went unnoticed for awhile because my application of stefon (https://github.com/RyanMcG/incise) only needed default the options in development and the once stage was in a single thread. Neither is the case anymore, and so I uncovered this issue with my original implementation which is fixed by binding the local var (*settings*) inside of the handler instead of around it (see wrap-options).

[boymaas]

Hi all, yes exactly. I followed the same logic as Ryan, expecting my settings to be present in the link-to-asset when specifying them in the middleware. It's ofcourse nice to be able override options in the link-to-asset, for whatever reason. But re-supplying those options through my whole application, in every link-to-assets call, doesn't feel right.

[RyanMcG]

ping

[pbiggar]

I think I'm going to close this one. I do agree that there is some problem, but I don't think making options optional is the solution. Perhaps making it clear in the README that you need to specify the options each time might be an option.

Sorry for the wild goose chase :(

[RyanMcG]

I don't think making options optional is the solution.

We're talking about removing options from link-to-asset, not making it optional.

Perhaps making it clear in the README that you need to specify the options each time might be an option.

The goal is for individuals to not have to redundantly specify options, not to be clearer that they must do it multiple times.

[pbiggar]

If you remove the options, you still end up in trouble in the multi-threaded case, right?

[RyanMcG]

No, that's what my most recent update was about (see wrap-options). https://github.com/RyanMcG/stefon/commit/3ee62be747a4c7ce08e8874136e087faed23680a

There we wrap inside of the handler generated by the middleware, instead of outside (so we are in the same thread). Of course, if a single request uses multiple threads...

Let's assume render calls link-to-asset.

This wouldn't work.

(defn handler [req]
  @(future (render req)))

Of course, this would.

(defn handler [req]
  (render req))

And this would too, since we are rebinding inside of the created thread.

(defn handler [req]
  (let [opts settings/*settings*]
    @(future (settings/with-options opts (render req)))))

[pbiggar]

Right, the multi-threaded behaviour is non-obvious in the with-options case.

[RyanMcG]

It's the result of using a dynamic var. If we do not need thread local bindings, then we could be using an atom (or another concurrent friendly way to mutate settings). I'm not sure how that would work out though.

It comes down to what provides a better interface. I do not have any stats on this, but it seems like the vast majority of use cases will work with the wrap-options middleware and removing the options parameter from link-to-asset in an obvious way that is DRY. The thread which renders the page is likely to be the same one as that handles the request. In the case that this is not so, the user is doing something weird so I do not think requiring a with-options is out of the question.

I suspect you will prefer the easier solution of always passing in options. I disagree with this not only for the DRY aspect, but it requires the user to make that configuration available in more places (loaded in there server and in there views). I've often used an atom storing that configuration (in case I want to be able to change it as the program runs). Perhaps, then using a dynamic var is not the right solution.

[boymaas]

I really do not understand what the problem is with merging this pull request. There are in my opinion no drawbacks. I needed to fork this repository to use stefon, as I refuse by principle to pass-in configuration options in every call to link-to-asset. And do this conditionally based on running in development or production modus. The big question is, what is exactly the objection? But heh, I'll keep on merging future changes on top of my fork ... glad there is at least something like stefon :+1: