Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

adding a "renderer" framework so assets can render themselves #283

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

adding a "renderer" framework so assets can render themselves #283

wants to merge 20 commits into from

Conversation

metagriffin
Copy link
Contributor

first, thanks for creating a great tool for managing assets!

one of the things that i found odd about webassets was the fact that when referencing an asset in a template, the template needs to be sensitive to the internals of the assets to be able to reference it correctly. for example, it had to know if it was css or javascript, if the less output was being compiled or not, etc.

this merge request is the beginnings of a solution that allows assets to render themselves using a renderer registry. for example, given the following bundle setup (notice that the bundle contains both css and javascript):

all_assets = Bundle(
  Bundle('style.css', output='app.css', renderer='css'),
  Bundle('script.js', output='app.js', renderer='js'))
env.register('app', all_assets)

knowing that the 'css' and 'js' renderers come baked-in, you can have the following mako template:

    <html>
      <head>
        % for asset in env['app'].renderers():
          ${asset.render()|n}
        % endfor
      </head>
      ...
    </html>

which will generate the following output:

    <html>
      <head>
        <link rel="stylesheet" type="text/css" href="/app.css"/>
        <script type="text/javascript" src="/app.js"></script>
      </head>
      ...
    </html>

to be yslow-optimized, you typically want styles defined at the top, and scripts at the bottom. this is possible with renderer "selection":

    <html>
      <head>
        ## only select renderers that are neither "js" nor "amd"
        % for asset in env['app'].renderers('!js,!amd'):
          ${asset.render()|n}
        % endfor
      </head>
      ...

        ## select renderers that are either "js" or "amd"
        % for asset in env['app'].renderers('js,amd'):
          ${asset.render()|n}
        % endfor

    </html>

asset contents can also be rendered inline via asset.render(inline=True).

existing renderers can be overridden and custom renderers added, eg:

env.register_renderer('meta-url', '<meta name="URL" content="{url}"/>')

the merge request has:

  • the full implementation for HTML referencing and inlining
  • unit tests for css and js renderers
  • documentation for how to use the renderer framework

please let me know if you agree that this would be a nice addition and/or what needs improvement.

thanks!

@metagriffin
Copy link
Contributor Author

hm, i've come to realize that the mixed-renderer case (provided in the example) only works when debug=true. that said, the renderers still work very well when all renderers in a bundle are the same, and accomplishes the goal of separation of concerns very well.

to address the mixed-renderer case, i believe the engine itself needs to be made more renderer-aware, and not merge assets with different renderers. do you agree?

@metagriffin
Copy link
Contributor Author

the renderer module is now mixed-renderer-enabled! while i was at it, i made it capable of renderer selection too (comment updated to demonstrate).

@miracle2k
Copy link
Owner

Sorry for the delay looking at this.

You've almost put too much work into this for me not to merge it :)

Here's the thing. The design of the template tags was a very conscious decision, but I've come to realize it was a mistake, although to me, one that is more fundamental than just a way to generate the html. For a long time I wanted to add a more high-level layer on top of what exists now (see #12), though these days, if I find the time, I suppose I'd rather start over with a webassets 2.

This looks like a solid patch. It obviously involves a bunch of design decisions, and unfortunately, I can't provide any input into those, because I don't feel very motivated to think about how to solve the problem in this context.

What I like about this a lot though is that is pretty encapsulated from the rest of the code. In fact, so much so that I wonder if it couldn't be a separate module, possibly "webassets.contrib". That would sort of relieve me of making a bigger decision here than I'm comfortable with.

Do you think this could work with only general purpose modifications to the core? I would imagine, for example, that renderers could be added to Bundle.extra, or to Environment.config, but without the property.

@metagriffin
Copy link
Contributor Author

hi @miracle2k, have you had a chance to think about this? (i need to upgrade various dependencies, and this is a conflict for me because i'm working off the branch)

IMHO, this belongs to webassets core because, well, how often does one use webassets outside of the context of HTML? i'd love to see this merged, but will only do the work of updating the branch to be mergeable if you're going to accept it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@metagriffin @miracle2k