Issue #82 new

Rollups might get wrongly placed bellow scripts which depends on one of it's components

Manuel Vázquez Acosta
created an issue

I have place a pull request with a test for this: https://bitbucket.org/fanstatic/fanstatic/pull-request/17/includes-a-new-regression-tests-rollups

There seems to be more for this error to happen. In my real-world app I removed the dependency link that causes the error in the test, and I still get the wrong behavior.

Comments (12)

  1. Jan-Jaap Driessen

    Cross library rollups are an interesting idea that Martijn and I didn't take into account when we implemented resource sorting. I understand the issue you point out in this ticket and am going to ask Martijn to chime in.

  2. Manuel Vázquez Acosta reporter

    I'm thinking (but I haven't complete the proof) that the supersedes relation is compatible with a "reversed" depends relation in the resources DAG, this is :

    If X --depends --> A And R -- supersedes --> (X, A)

    On may introduce R as node in the dag like this:

    X ---> A ---> R
     +---------------^
    

    This way if a Y depends on A, it will catch the R dependency.

    I will keep working in this idea to see if it's right, and then look a your code to see if it's easy to implement (the only difficulty I foresee is that R should not always be in the list of resources but it'll be there iff all its supersedees are needed).

  3. Manuel Vázquez Acosta reporter

    I think the idea before is right. Logically no resource should depend directly on a rollup but one of it's components. So rollups are a kind of after-the-fact from a dependency-graph point of view. They could be introduced in this graph the way I've described before: A rollup should never have an outgoing arrow in the dependency DAG.

    Being that established I realized that you don't use in the NeededResources.resources() method a topological sort, but a normal sort based on a key that uses a dependency "depth" notion (init_library_nr, and init_dependency_nr methods).

    I see you have a sort_resource_topological method, but it seems you're not using it for resources sorting. Am I correct?

  4. Manuel Vázquez Acosta reporter

    I have created a patch that solves both my real-life problem and the test I have placed in my pull request.

    It's rather hackish that's why I haven't done a commit + pull request.

    You can see it in https://gist.github.com/mvaled/5390968

    Since consolidate is used only when getting all the resources, I have place a simple hack there: if a resource is being placed as-is and it has a dependency that was replaced by a rollup, add the rollup itself as a dependency of the resource.

    Hope this is not too wrong.

    Waiting for your review, Manuel.

  5. faassen

    Manuel, cool that you're looking into this! I am currently at a sprint on Ibiza (doing work on Obviel) so I don't have a lot of time for commenting.

    I remember we indeed don't use sort_resource_topological; I think I had to introduce it for another feature at the time, I think helping to generate Python code automatically from YUI's dependency graph. It's pretty old and the dependency depth story is newer.

  6. faassen

    Concerning the patch it'd be great if you could include some test cases. They're not hard to write with py.test and you can expose the problem clearly.

  7. Jan-Jaap Driessen

    Here's an idea for bundling javascript across libraries, perhaps this feature would remove the need for Manuel's rollup.

    Let's say we introduce a feature of cross-library bundling:

    We have a resource foo.js in library "bar". This resource foo.js depends on js.jquery.jquery.

    If you configure fanstatic to do cross library bundling, these two resources could be served in one request, something like:

    /fanstatic/:cross-bundle:^jquery:version:1.9.1/jquery.js^bar:version:1.0/foo.js

    (Where ^ is the character I came up with to differentiate between libraries in a cross-bundle)

    The name "megabundle" is the first that comes to mind, but I am still looking for other options.

    (For CSS we can only do this if a Resource is "bundle-safe" which means, has no relative paths to other resources, such as images, fonts.)

    Manuel, would this feature help in your situation?

  8. Manuel Vázquez Acosta reporter

    faasen, I have provided a pull request with a test for that, and is currently passing with my patch applied. Maybe you require more tests I haven't taken into account.


    Jan-Jaap, Probably the megabundle for JS works in some cases. But I wouldn't want it everywhere. Since from a global application perspective (taking account of chaching and the like) some libraries would make more sense to serve by themselves and never bundle. But some 2 o 3 pieces of JS you use almost everywhere do make sense to bundle.

    I would love more an "automatic" rollup build. Something that would be like a current Rollup plus Bundle. That would keep the benefit to keeping in control (what you can cross-library bundle), and remove the hurdle of maintaining the rollup by hand.

    That would be like AutoRollup(from=[js.jquery.jquery, bootstrap_js, my_app_js]).

    What do you think?

  9. Manuel Vázquez Acosta reporter

    faassen

    I have updated my pull request to the current 1.0a and also included my patch. Using this version the bug no longer shows in my env. But I can't be sure it will work smoothly with slots and other stuff I don't really understand just yet.

    Best regards, manuel.

  10. Jan-Jaap Driessen

    I was discussing fanstatic bundling with some people last week and they also came up with the use case of a predefined rollup of resources that "I know will be rendered together". I'll give this some more thought and look at Manuel's code. I am particularly interested in how this works with bundling.

  11. Log in to comment