Feature/issue 117

[eadmundo]

First pass at versioned images as described in #117 - thoughts and comments very welcome

[miracle2k]

(workspace)michael@ubuntu:~/Development/workspace/webassets117/media$ cat out.css 
body {
   background-image: url('out/icon.d41d8cd9.png');
}

It works! :)

Mainly, I would like to change the API of the ExternalAssets class to match that of Bundle more closely. This makes sense on a number of levels, I think.

First, there will be a need to have multiple external asset definitions. For example, different application modules contributing stuff. To that end, if ExternalAssets, instead of write_files, would expose a build method that is compatible with Bundle.build, then it could just be registered with the environment like any other bundle; potentially, no special handling would be required in places like script.py.

Second, the file specification of ExternalAssets should match that of the Bundle class. There is probably some potential for code sharing here, i.e. moving code like Bundle.resolve_contents() to a shared "Container" class. Certainly, it would be necessary for ExternalAssets to also call into the _normalize_source_path hooks so that third party extensions like Flask-Assets can support references within their modules.

Also, I would suggest that we rename webassets.externalassets to just webassets.external. It's just a bit leaner.

[eadmundo]

I've started to change the API for ExternalAssets to be more like Bundle, so it can now be registered as if it were just another bundle. Still some more to do there, though.

I had to change the cssrewrite filter (which is one of the key parts of this to me, in terms of what I need it for at least...) so that the bundle being built can get knowledge of the ExternalAssets object required. To do this I added another keyword argument for the filter, external. This means having to call the filter using get_filter (like this) , which I'm not delighted about, but I haven't thought of a better way to do that yet.

As before, all comments welcomed!

[miracle2k]

I thought that the cssrewrite filter would just loop through the bundles known to the environment, filter out the ExternalAssets instances, and then work with all of them. That is, the first ExternalAssets instance that finds the file in question is used.

So I might be able to do:

  env.register(ExternalAssets("app1/**/*.png"))
  env.register(ExternalAssets("app2/**/*.png"))

  env.register('input.css', filters='cssrewrite', output='out.css')

And input.css could reference both files from app1 or app2, and the correct ExternalAssets instance is used.

That means that in larger applications, different parts can add their own external assets. Also, it would potentially allow for things like filters (e.g. thumbnail creation) to be added in the future, which may only be needed on some files.

[miracle2k]

I've merged your branch against the current master (https://github.com/miracle2k/webassets/tree/feature/issue-117); In master, significant changes have been made since when you branched off, so I would encourage you to work with the latest version.

[travisbot]

This pull request passes (merged 165fd7b into ef4d11e).

[tgecho]

I'm trying out this fork and it seems to work really well on initial trial. I've only used a simple bundle (ExternalAssets('img/*', output='img')) plus the cssrewrite filter on my css files so far, but the basics seem to work. Nice work!

I assume at this point we're looking at needing tests (plus the fixes found in developing them), docs and a few scraps of time to work on them? :)

[tgecho]

So much code write, so little time! I'll admit to being confused by the
webassets testing infrastructure at first glance, but I'll try to find
some time to draft some docs. Do you have a preference for reporting
issues on this pull request that I can't come up with my own patch for?

[eadmundo]

no preference at all really, whatever works for you.

[miracle2k]

Guys, this seems to be coming along quite nicely. I need this now myself, so I'll have a closer look and hopefully we can get it finalized and merged soon.

[eadmundo]

That would be great. Apologies for the lack of activity on this - started a new job, so time has been limited!

[tilgovi]

I think I fixed this for myself in my pyramid_webassets fork 0 simply by making the resolver's resolve_source_to_url compute a version. It's a little backwards because the Bundle isn't known at that point (so I create a temporary one). It seems like the source of the problem is that Bundle._urls() in 0.8 calls self._make_output_url for any built bundles but a parallel action is not performed when there is nothing to build. If the source URLs were versioned the same way, the whole problem would disappear.

As an aside, I leverage this in my app now to version assets referenced in the CSS 1. Is this something you'd like to add to the CSSRewrite filter (or add upstream as its own filter)?

[tgecho]

I realize everyone (including myself) is quite busy, but I was wondering about the general sentiment on this branch. Obviously it doesn't merge cleanly into master right now. On top of that, is it more a matter of design decisions or just time to polish/test? I'd like to help if I can. It's a shame to see such useful code languishing :)

@eadmundo @miracle2k

[tilgovi]

I would really appreciate if anyone could respond to my comment. I managed to get versioned images just fine.

[eadmundo]

Hi @tilgovi

sorry for the lack of response! As you can see this feature hasn't had a lot of time spent on it recently, sadly. Personally I would definitely want to add versioning for assets referenced in the CSS, but having not looked at the code for a while it's hard for me to say whether it should be in CSSRewrite or its own filter.

@tgecho I think there are some design decisions to be made still, and certainly a lot of polishing and testing. Also I would think it's now a fair way behind master, so much work remains to be done. Unfortunately I have not had any time to work on this due to changing jobs and using a different system for static assets, but obviously feel free to dive in!

Ed