Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: core, extras
    • Labels:
      None
    • Environment:
      Click 1.5

      Description

      Hi Malcolm,

      Here are a list of enhancements I think would make PerformanceFilter more newbie friendly:

      #1 It would be nice to leave the PerformanceFilter in web.xml for development mode and do not need to change the web.xml file when deploying to production. However the Filter always caches, thus while editing 'css' and 'js' files we need to forcefully refresh the browser to ensure the cache is flushed.

      So my suggestion is that if application mode is below 'profile', PerformanceFilter should not use caching.

      #2 Currently Click adds a version to 'css', 'js' and image files. This might seem surprising for new user. I am worried this will confuse them, since performance won't be a worry in the early stages. I have been reading up on browser caching and it seems that the filename is used to cache the resource. If the contents of the file changes the browser won't reload. Only when the filename changes will it reload.

      So the suggestion is to let the PageImports add the version info only in production/profile mode. In production modes the 'js', 'css' and images would have versions appended to them. Since these resources do not actually exist on the server, the PerformanceFilter must strip of the version info from the path and forward (using RequestDispatcher) these requests to their real locations.

      I made a quick prototype of #2 to ensure it does in fact work.

      If these suggestions are approved I can upload a patch if needed.

      regards

      bob

        Activity

        Hide
        Malcolm Edgar added a comment -

        I am pretty happy with the way this is working now good work Bob.

        regards Malcolm Edgar

        Show
        Malcolm Edgar added a comment - I am pretty happy with the way this is working now good work Bob. regards Malcolm Edgar
        Hide
        Malcolm Edgar added a comment -

        Hi Bob,

        OK now I understand #1, good pickup. That would have been a nasty bug.

        regards Malcolm Edgar

        Show
        Malcolm Edgar added a comment - Hi Bob, OK now I understand #1, good pickup. That would have been a nasty bug. regards Malcolm Edgar
        Hide
        Bob Schellink added a comment -

        Hi Malcolm,

        I agree. If a advanced users wants to define versioning they can do so themselves. Also PerformanceFitler javadoc should be trimmed since its too much blabbering

        However #1is not about disabling versioning for particular requests. Let me explain why I put it in.

        Originally I did not have the attribute defined. In development modes the version indicator was not added, while in production modes the version was added. I was about to check this in when I realized that a user might not actually define the PerformanceFilter in web.xml at all. So by commenting out the PerformanceFilter from web.xml the resources was not found anymore, because the resource version was still added but there was no filter to remove the version so that the real resource can be served.

        My understanding is that PerformanceFilter is an optional filter that a user can apply if he/she wants.

        If however the PerformanceFilter must be defined then you are correct and the attribute is not needed since the filter will always be there to strip the version indicator.

        Let me know if I am not making sense

        Show
        Bob Schellink added a comment - Hi Malcolm, I agree. If a advanced users wants to define versioning they can do so themselves. Also PerformanceFitler javadoc should be trimmed since its too much blabbering However #1is not about disabling versioning for particular requests. Let me explain why I put it in. Originally I did not have the attribute defined. In development modes the version indicator was not added, while in production modes the version was added. I was about to check this in when I realized that a user might not actually define the PerformanceFilter in web.xml at all. So by commenting out the PerformanceFilter from web.xml the resources was not found anymore, because the resource version was still added but there was no filter to remove the version so that the real resource can be served. My understanding is that PerformanceFilter is an optional filter that a user can apply if he/she wants. If however the PerformanceFilter must be defined then you are correct and the attribute is not needed since the filter will always be there to strip the version indicator. Let me know if I am not making sense
        Hide
        Malcolm Edgar added a comment -

        Hi Bob,

        Thanks for the feedback.

        #1 - OK now I understand the intent is to disable resource versioning for a particular resource request. I think this is a fringe use case, and I don't really see a need for this. One of my objectives with the Click code base is to keep it as small as possible, as there is less code to maintain, understand, debug and document (with documentation being a pretty onerous but important job).

        #2 - OK, I can see your potential use case for this. However I think it would be for pretty hardcore Click users and for very performance demanding sites. Given this I would prefer to lower this features visibility, so not as to overwhelm people looking to learn how to use the PerformanceFilter. To do this, as you were alluding to, I think custom Control developers should just implement the getHtmlImports() method for themselves to insert custom version information, and not add the AbstractControl getResourceVersionIndicator() method. The ClickUtils.createHtmlImport() method would then revert back to its previous signature.

        I would leave the protected methods you have defined in the PerformanceFilter for people to override if they take a real interest, but I would not bother documenting those extension point feature in the header Javadoc.

        regards Malcolm Edgar

        Show
        Malcolm Edgar added a comment - Hi Bob, Thanks for the feedback. #1 - OK now I understand the intent is to disable resource versioning for a particular resource request. I think this is a fringe use case, and I don't really see a need for this. One of my objectives with the Click code base is to keep it as small as possible, as there is less code to maintain, understand, debug and document (with documentation being a pretty onerous but important job). #2 - OK, I can see your potential use case for this. However I think it would be for pretty hardcore Click users and for very performance demanding sites. Given this I would prefer to lower this features visibility, so not as to overwhelm people looking to learn how to use the PerformanceFilter. To do this, as you were alluding to, I think custom Control developers should just implement the getHtmlImports() method for themselves to insert custom version information, and not add the AbstractControl getResourceVersionIndicator() method. The ClickUtils.createHtmlImport() method would then revert back to its previous signature. I would leave the protected methods you have defined in the PerformanceFilter for people to override if they take a real interest, but I would not bother documenting those extension point feature in the header Javadoc. regards Malcolm Edgar
        Hide
        Bob Schellink added a comment -

        A use case for #2 would be for writing a custom control and using ClickUtils#createHtmlImport(...) to insert the version indicator. This will insert the Click version into the resource path.

        But a custom application will have a faster release cycle than Click. So if a new release of the application is rolled out a week later, createHtmlImport(...) would still append Click version. The browser will still serve the cached version.

        Perhaps custom controls shouldn't use createHtmlImport for versioning, but rather implement their own version?

        Show
        Bob Schellink added a comment - A use case for #2 would be for writing a custom control and using ClickUtils#createHtmlImport(...) to insert the version indicator. This will insert the Click version into the resource path. But a custom application will have a faster release cycle than Click. So if a new release of the application is rolled out a week later, createHtmlImport(...) would still append Click version. The browser will still serve the cached version. Perhaps custom controls shouldn't use createHtmlImport for versioning, but rather implement their own version?
        Hide
        Bob Schellink added a comment -

        Hi Malcolm,

        #2. I added the attribute because if the PerformanceFilter is not defined in web.xml (say a user does not want the filter for some reason), then createHtmlImport(...) will add the the version indicator in production or profile mode. But the filter is not there to strip the version indicator from the path and the resource is not found. So the attribute acts as a way to disable versioning even in production modes.

        #1 I assume you mean it is speculative that users would override the method getResourceVerionIndicator? I agree that at this stage it is overkill. However the method AbstractControl#getResourceIndicator is protected and non-final so can be overriden. I thought the intention was that users could provide their own implementation. If not perhaps we should not have the method AbstractControl#getResourceVersionIndicator but rather use ClickUtils.getResourceVersionIndicator.

        With the above changes hopefully we can remove some of the javadoc of PerformanceFilter. I ended having to write quite a bit more than I would have liked

        Show
        Bob Schellink added a comment - Hi Malcolm, #2. I added the attribute because if the PerformanceFilter is not defined in web.xml (say a user does not want the filter for some reason), then createHtmlImport(...) will add the the version indicator in production or profile mode. But the filter is not there to strip the version indicator from the path and the resource is not found. So the attribute acts as a way to disable versioning even in production modes. #1 I assume you mean it is speculative that users would override the method getResourceVerionIndicator? I agree that at this stage it is overkill. However the method AbstractControl#getResourceIndicator is protected and non-final so can be overriden. I thought the intention was that users could provide their own implementation. If not perhaps we should not have the method AbstractControl#getResourceVersionIndicator but rather use ClickUtils.getResourceVersionIndicator. With the above changes hopefully we can remove some of the javadoc of PerformanceFilter. I ended having to write quite a bit more than I would have liked
        Hide
        Malcolm Edgar added a comment -

        Hi Bob,

        Saw your checkins, they look good.

        Regarding #2 - adding the request attribute, is this to support the forwarding of a versioned resource to the PerformanceFilter?

        With #1 - I think this feature is a bit speculative, and I am worrying about making a complex area of Click even more complex (well relative for Click anyway). Do you have a particular use case in mind where you will be applying this?

        regards Malcolm Edgar

        Show
        Malcolm Edgar added a comment - Hi Bob, Saw your checkins, they look good. Regarding #2 - adding the request attribute, is this to support the forwarding of a versioned resource to the PerformanceFilter? With #1 - I think this feature is a bit speculative, and I am worrying about making a complex area of Click even more complex (well relative for Click anyway). Do you have a particular use case in mind where you will be applying this? regards Malcolm Edgar
        Hide
        Bob Schellink added a comment -

        I have changed createHtmlImport signature to:

        public void createHtmlImport(String pattern, String versionIndicator, Context context);

        So createHtmlImport will use the versionIndicator passed as a argument, instead of generating one. Otherwise one would not be able to change the version indicator if needed. For example one might want to use the applications version number instead of Click's version number. Or the resources last modified date. Another popular version indicator is the .svn version numbers.

        With the above change one can override the method AbstractControl#getResourceVersionIndicator and return an alternative version number.

        Show
        Bob Schellink added a comment - I have changed createHtmlImport signature to: public void createHtmlImport(String pattern, String versionIndicator, Context context); So createHtmlImport will use the versionIndicator passed as a argument, instead of generating one. Otherwise one would not be able to change the version indicator if needed. For example one might want to use the applications version number instead of Click's version number. Or the resources last modified date. Another popular version indicator is the .svn version numbers. With the above change one can override the method AbstractControl#getResourceVersionIndicator and return an alternative version number.
        Hide
        Bob Schellink added a comment -

        I will add javadoc soon.

        Show
        Bob Schellink added a comment - I will add javadoc soon.
        Hide
        Bob Schellink added a comment -

        Ok, PerformanceFilter checked in. Works pretty well. A couple of notes:

        #1 Renamed the Context.getClickVersion to Context.getResourceVersionIndicator. Also added ClickUtils.getResourceIndicator that caches the indicator so no string concatenation is needed.
        #2 Added a request attribute to indicate if getHtmlImports should apply resource versioning. This attribute is set in PerformanceFilter when mode if profile or production. Thus resource versioning will only occur if PerformanceFilter is active and Click is set to profile or production mode. Of course users can still enable versioning by setting the request attribute themselves.

        Show
        Bob Schellink added a comment - Ok, PerformanceFilter checked in. Works pretty well. A couple of notes: #1 Renamed the Context.getClickVersion to Context.getResourceVersionIndicator. Also added ClickUtils.getResourceIndicator that caches the indicator so no string concatenation is needed. #2 Added a request attribute to indicate if getHtmlImports should apply resource versioning. This attribute is set in PerformanceFilter when mode if profile or production. Thus resource versioning will only occur if PerformanceFilter is active and Click is set to profile or production mode. Of course users can still enable versioning by setting the request attribute themselves.
        Hide
        Malcolm Edgar added a comment -

        Hi Bob,

        Yes I thought about exposing the modes in context, but as its an internal thing I didn't think there was much point.

        An easy was to do this is:

        if (getContext().getApplicationMode().startsWith("pro") {

        } else {

        }

        Show
        Malcolm Edgar added a comment - Hi Bob, Yes I thought about exposing the modes in context, but as its an internal thing I didn't think there was much point. An easy was to do this is: if (getContext().getApplicationMode().startsWith("pro") { } else { }
        Hide
        Bob Schellink added a comment -

        Hi Malcolm,

        I wonder if we should not expose the application mode as an int as well as a string. Currently inside ClickApp we can do the following:

        if (mode >= DEVELOPMENT)

        { //do something }

        However ClickApp is not a public API and the rest of the framework must use code like this:

        if ("development".equals(getContext().getApplicationMode())

        "debug".equals(getContext().getApplicationMode())
        "trace".equals(getContext().getApplicationMode())) { //do something }

        I used the above code in PerformanceFilter, but am thinking of copying ClickApp's static int values to the Filter to make this easier.

        An ideal place for this would a ClickConfig type class or perhaps Context.

        For now I'll just copy the ClickApp code. Feel free to veto this

        Show
        Bob Schellink added a comment - Hi Malcolm, I wonder if we should not expose the application mode as an int as well as a string. Currently inside ClickApp we can do the following: if (mode >= DEVELOPMENT) { //do something } However ClickApp is not a public API and the rest of the framework must use code like this: if ("development".equals(getContext().getApplicationMode()) "debug".equals(getContext().getApplicationMode()) "trace".equals(getContext().getApplicationMode())) { //do something } I used the above code in PerformanceFilter, but am thinking of copying ClickApp's static int values to the Filter to make this easier. An ideal place for this would a ClickConfig type class or perhaps Context. For now I'll just copy the ClickApp code. Feel free to veto this
        Hide
        Malcolm Edgar added a comment -

        Checked in non PerformanceFilter changes to support this item:

        • ClickUtils.createHtmlImport() now will insert the click version string if in profile or production mode. Note this is commented out.
        • Controls now deploy static resources without any version string
        • Controls HTML import strings updates to support ClickUtils.createHtmlImport() method
        Show
        Malcolm Edgar added a comment - Checked in non PerformanceFilter changes to support this item: ClickUtils.createHtmlImport() now will insert the click version string if in profile or production mode. Note this is commented out. Controls now deploy static resources without any version string Controls HTML import strings updates to support ClickUtils.createHtmlImport() method
        Hide
        Malcolm Edgar added a comment -

        Hi Bob,

        They sound like greate enhancements. I have assigned this improvement to you, so you can check in the changes directly.

        Please add some Javadoc to the PerformanceFilter, detailing how this work.

        regards Malcolm Edgar

        Show
        Malcolm Edgar added a comment - Hi Bob, They sound like greate enhancements. I have assigned this improvement to you, so you can check in the changes directly. Please add some Javadoc to the PerformanceFilter, detailing how this work. regards Malcolm Edgar
        Hide
        Bob Schellink added a comment -

        > "So the suggestion is to let the PageImports add the version info only in production/profile mode."

        Made a mistake in the above statement. The Controls themselves adds the version, not PageImports.

        Show
        Bob Schellink added a comment - > "So the suggestion is to let the PageImports add the version info only in production/profile mode." Made a mistake in the above statement. The Controls themselves adds the version, not PageImports.

          People

          • Assignee:
            Bob Schellink
            Reporter:
            Bob Schellink
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development