Wicket
  1. Wicket
  2. WICKET-4161

AbstractResourceAggregatingHeaderResponse does not order javascript properly

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5.1
    • Fix Version/s: 1.5.3
    • Component/s: wicket

      Description

      I decided to give AbstractResourceAggregatingHeaderResponse a try but I found a problem in its design that, I think, makes it unusable.

      How it works:
      1-the IHeaderResponse#renderJavaScriptReference(ResourceReference) methods are intercepted and the resource references are stored in a list and not rendered right away. Each resource ref is assigned a key for grouping.

      2-When close() is called on the response, the aggregator writes out all the accumulated resource references which have not been rendered yet. This step can be used to group multiple resources into a single merged resource.

      The problem:
      AbstractResourceAggregatingHeaderResponse does not intercept calls to IHeaderResponse#renderJavaScriptReference(url) or IHeaderResponse#renderJavaScript(Script). They are directly executed by the underlying response.

      example:
      AbstractDefaultAjaxBehavior#renderHead() does the following :
      response.renderJavaScriptReference(WicketEventReference.INSTANCE);
      response.renderJavaScriptReference(WicketAjaxReference.INSTANCE);
      response.renderJavaScript("Wicket.Ajax.baseUrl=[...]");

      With the non-aggregating header response, the Wicket .js ref script tags are rendered in the markup before the inline javascript code and all is well.
      With the aggregating version, the Wicket js resource references are rendered last (in close()). This means that the inline javascript code (which uses Wicket.Ajax) is executed before the Wicket .js files are loaded, causing a javascript error (Wicket is undefined).

      This problem also applies to css resource references because order of inclusion is important for them too.

      Short of a big refactor to force each rendered javascript to list its dependencies, I don't see how this problem can be solved. I opened this ticket primarily to share my findings and let people comment on possible solutions other than removing the code.

      The problem is also present in AbstractDependencyRespectingResourceAggregatingHeaderResponse.

      NOTE:
      wiQuery uses AbstractResourceAggregatingHeaderResponse, but resolves the issue by intercepting all renderJavaScript* methods and keeping their order.

      1. WICKET-4161.zip
        26 kB
        Bertrand Guay-Paquet
      2. WICKET-4161.patch
        17 kB
        Martin Grigorov
      3. quickstart2.patch
        1 kB
        Bertrand Guay-Paquet

        Activity

        Hide
        Martin Grigorov added a comment -

        I think this should be easy to solve.
        We just need to extend org.apache.wicket.resource.aggregation.ResourceReferenceAndStringData to work with either ResourceReference or url (String),
        Then org.apache.wicket.resource.ResourceUtil.renderTo(IHeaderResponse, ResourceReference, boolean, String) delegating to org.apache.wicket.resource.ResourceUtil.renderTo(IHeaderResponse, ResourceReference, boolean, String) will use the proper method of org.apache.wicket.markup.html.IHeaderResponse to render the ResourceReference or String.

        Please create a quickstart that shows the problem and I'll see whether my approach will work.

        Show
        Martin Grigorov added a comment - I think this should be easy to solve. We just need to extend org.apache.wicket.resource.aggregation.ResourceReferenceAndStringData to work with either ResourceReference or url (String), Then org.apache.wicket.resource.ResourceUtil.renderTo(IHeaderResponse, ResourceReference, boolean, String) delegating to org.apache.wicket.resource.ResourceUtil.renderTo(IHeaderResponse, ResourceReference, boolean, String) will use the proper method of org.apache.wicket.markup.html.IHeaderResponse to render the ResourceReference or String. Please create a quickstart that shows the problem and I'll see whether my approach will work.
        Hide
        Bertrand Guay-Paquet added a comment -

        quickstart

        Show
        Bertrand Guay-Paquet added a comment - quickstart
        Hide
        Martin Grigorov added a comment -

        Here is a patch that fixes the problem.
        It has a small API break in the constructor of ResourceReferenceAndStringData but I think this code deserves even more work.

        Check it and let us know whether it works for your real app.
        I'll ask Hielke (WiQuery) also for review.
        I'm not sure how many people actually use this code but I doubt there are a lot.

        Show
        Martin Grigorov added a comment - Here is a patch that fixes the problem. It has a small API break in the constructor of ResourceReferenceAndStringData but I think this code deserves even more work. Check it and let us know whether it works for your real app. I'll ask Hielke (WiQuery) also for review. I'm not sure how many people actually use this code but I doubt there are a lot.
        Hide
        Bertrand Guay-Paquet added a comment -

        patch for quickstart showing grouping problem

        Show
        Bertrand Guay-Paquet added a comment - patch for quickstart showing grouping problem
        Hide
        Bertrand Guay-Paquet added a comment -

        Your patch does fix the problem if grouping is only done by aggregating css and js in 2 groups. However, the AbstractResourceAggregatingHeaderResponse is still vulnerable to whatever aggregating method given by its subclass. My last quickstart patch shows this problem. Of course, the newGroupingKey() method I used was artificially manufactured explicitly to trigger the problem, but it shows that the aggregating functionality can easily break the required ordering of resources.

        From my understanding, the subclass AbstractDependencyRespectingResourceAggregatingHeaderResponse of AbstractResourceAggregatingHeaderResponse is designed to overcome this issue by using ResourceDependentResourceReference. However, no part of Wicket currently uses the latter so a simple dependent ordering like the one in AbstractDefaultAjaxBehavior#renderHead() is still vulnerable.

        Show
        Bertrand Guay-Paquet added a comment - Your patch does fix the problem if grouping is only done by aggregating css and js in 2 groups. However, the AbstractResourceAggregatingHeaderResponse is still vulnerable to whatever aggregating method given by its subclass. My last quickstart patch shows this problem. Of course, the newGroupingKey() method I used was artificially manufactured explicitly to trigger the problem, but it shows that the aggregating functionality can easily break the required ordering of resources. From my understanding, the subclass AbstractDependencyRespectingResourceAggregatingHeaderResponse of AbstractResourceAggregatingHeaderResponse is designed to overcome this issue by using ResourceDependentResourceReference. However, no part of Wicket currently uses the latter so a simple dependent ordering like the one in AbstractDefaultAjaxBehavior#renderHead() is still vulnerable.
        Hide
        Martin Grigorov added a comment -

        I think this is responsibility of the developer to provide proper implementation of org.apache.wicket.resource.aggregation.AbstractResourceAggregatingHeaderResponse.newGroupingKey(ResourceReferenceAndStringData).

        The problem that Wicket itself doesn't use these classes should be discussed in another ticket.
        As I wrote in my mail to dev@ yesterday: we should decide which way to go: improve the classes in o.a.w.resource.aggregation or use JavaScript library that does this. For example RequireJS.

        Show
        Martin Grigorov added a comment - I think this is responsibility of the developer to provide proper implementation of org.apache.wicket.resource.aggregation.AbstractResourceAggregatingHeaderResponse.newGroupingKey(ResourceReferenceAndStringData). The problem that Wicket itself doesn't use these classes should be discussed in another ticket. As I wrote in my mail to dev@ yesterday: we should decide which way to go: improve the classes in o.a.w.resource.aggregation or use JavaScript library that does this. For example RequireJS.
        Hide
        Martin Grigorov added a comment -

        The problem in this ticket is solved.
        Any other issues should be discussed in separate tickets.

        Show
        Martin Grigorov added a comment - The problem in this ticket is solved. Any other issues should be discussed in separate tickets.

          People

          • Assignee:
            Martin Grigorov
            Reporter:
            Bertrand Guay-Paquet
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development