Moodle
  1. Moodle
  2. MDL-41878

Change YUI loader to use shorter path for Moodle modules

    Details

    • Testing Instructions:
      Hide
      • Open various pages in Moodle
      • Check that all JS continues to work as expected
      • Confirm that there were no (new) JS errors displayed
      • *Confirm that the URL when loading modules is now more like
        m/-1/core/popuphelp/popuphelp.js
      Show
      Open various pages in Moodle Check that all JS continues to work as expected Confirm that there were no (new) JS errors displayed *Confirm that the URL when loading modules is now more like m/-1/core/popuphelp/popuphelp.js
    • Affected Branches:
      MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull Master Branch:
      MDL-41878-master
    • Rank:
      53046

      Description

      There's a max length of approx 1024 characters in the querystring by the YUI loader - this is to prevent issues caused by limitations imposed by various web servers.

      When a URL gets to 1024 characters, the loader breaks it up into multiple queries and issues a new request. So for lots of long module names, you end up more queries.

      At the moment, we give all Moodle modules a path of:

      moodle/$jsrev/component/modname/submodname.(js|css)

      and a similar path for images.

      Since we control the namespaces, and we know that we currently use paths of:

      • moodle
      • 2in3
      • gallery
      • 3.9.1 (or other version number) = yui3

      We can make 'm' an alias for 'moodle' and trim 5 chars off each of our module requests.

      It doesn't seem like much, but it can mean fewer requests and it's a cheap/easy thing to implement.

        Activity

        Hide
        Andrew Nicols added a comment -

        Petr Škoda and Sam Hemelryk, you may be particularly interested in this.

        I've set this to also allow 'moodle' so this shouldn't break anyone calling modules directly and bypassing the loader.

        Show
        Andrew Nicols added a comment - Petr Škoda and Sam Hemelryk , you may be particularly interested in this. I've set this to also allow 'moodle' so this shouldn't break anyone calling modules directly and bypassing the loader.
        Hide
        Petr Škoda added a comment -

        this has my +1, up to Sam Hemelryk for final decision during integration...

        Show
        Petr Škoda added a comment - this has my +1, up to Sam Hemelryk for final decision during integration...
        Hide
        Sam Hemelryk added a comment -

        Hmmm I am no opposed to the idea having a shortened version of Moodle for these requests.
        I do have a couple of questions though Andrew:

        1. You mention it can lead to fewer requests, I may have missed something but how is that?
        2. Would it not be better to do something like if ($version === 'm') $version = 'moodle' to treat it more like an alias and to remove the need to check if $version is m || moodle later on (plus the places where you've changed $version => 'moodle'?
        Show
        Sam Hemelryk added a comment - Hmmm I am no opposed to the idea having a shortened version of Moodle for these requests. I do have a couple of questions though Andrew: You mention it can lead to fewer requests, I may have missed something but how is that? Would it not be better to do something like if ($version === 'm') $version = 'moodle' to treat it more like an alias and to remove the need to check if $version is m || moodle later on (plus the places where you've changed $version => 'moodle'?
        Hide
        Andrew Nicols added a comment -

        Hi Sam,

        1. There's a maximum request length that the Loader imposes of 1,024 characters. That's because some web servers will throw a 500 Internal Server Error when supplied with a longer query. The client has no way to determine what the maximum length on any random server may be to make this longer or shorter for alternative cases. By shortening the version by five characters, in situations where we're currently just over the limit, we may be able to get more file requests into a single query, thus reducing the number of queries in total. This is unlikely to make a massive difference, but when combined with some other proposed changes to introduce hashed module names in upstream YUI, this could actually make a bigger difference in the future.

        2. That's a good idea - would you like me to implement that now and squash the commits?

        Thanks,

        Andrew

        Show
        Andrew Nicols added a comment - Hi Sam, 1. There's a maximum request length that the Loader imposes of 1,024 characters. That's because some web servers will throw a 500 Internal Server Error when supplied with a longer query. The client has no way to determine what the maximum length on any random server may be to make this longer or shorter for alternative cases. By shortening the version by five characters, in situations where we're currently just over the limit, we may be able to get more file requests into a single query, thus reducing the number of queries in total. This is unlikely to make a massive difference, but when combined with some other proposed changes to introduce hashed module names in upstream YUI, this could actually make a bigger difference in the future. 2. That's a good idea - would you like me to implement that now and squash the commits? Thanks, Andrew
        Hide
        Sam Hemelryk added a comment -

        Aha gotcha makes sense how this could potentially reduce the number of requests then.

        Could you please make that change when you are online and squash. If you could do that before this time tomorrow that would be great Andrew.
        You must be up early, still adjusting to the timezone change? how did the move go?

        Show
        Sam Hemelryk added a comment - Aha gotcha makes sense how this could potentially reduce the number of requests then. Could you please make that change when you are online and squash. If you could do that before this time tomorrow that would be great Andrew. You must be up early, still adjusting to the timezone change? how did the move go?
        Hide
        Andrew Nicols added a comment -

        I've made the suggested changes, and pushed the amended (and rebased) commit.

        I've also included an example the expected output as screenshots. With my hostname this does indeed lead to a reduction in the number of requests on the front page because popuphelp-min.js is now able to fit into the previous request.

        Show
        Andrew Nicols added a comment - I've made the suggested changes, and pushed the amended (and rebased) commit. I've also included an example the expected output as screenshots. With my hostname this does indeed lead to a reduction in the number of requests on the front page because popuphelp-min.js is now able to fit into the previous request.
        Hide
        Andrew Nicols added a comment -

        p.s. I've started on a proof of concept for repeatably hashed modulenames which means that in most circumstances, all moodle module names are 17 characters after the jsrev:

        http://loganberry.local/stable_master/theme/yui_combo.php?3.12.0/escape/escape-min.js&moodle/1380596269/core/dock/dock-loader-min.js
        

        Is hashed (with unique constraints) to:

        http://loganberry.local/stable_master/theme/yui_combo.php?3.12.0/escape/escape-min.js&moodle/1380596735/mh/d8ecd4-min.js
        

        Just a theory to play with but may be useful later.

        Show
        Andrew Nicols added a comment - p.s. I've started on a proof of concept for repeatably hashed modulenames which means that in most circumstances, all moodle module names are 17 characters after the jsrev: http://loganberry.local/stable_master/theme/yui_combo.php?3.12.0/escape/escape-min.js&moodle/1380596269/core/dock/dock-loader-min.js Is hashed (with unique constraints) to: http://loganberry.local/stable_master/theme/yui_combo.php?3.12.0/escape/escape-min.js&moodle/1380596735/mh/d8ecd4-min.js Just a theory to play with but may be useful later.
        Hide
        Andrew Nicols added a comment -

        Actually - that hash can be at least halved. Every character counts

        Show
        Andrew Nicols added a comment - Actually - that hash can be at least halved. Every character counts
        Hide
        Sam Hemelryk added a comment -

        Thanks Andrew - this has been integrated now.

        Show
        Sam Hemelryk added a comment - Thanks Andrew - this has been integrated now.
        Hide
        Ankit Agarwal added a comment -

        Scripts were named as indicated.
        Tested a few random js actions for regressions, couldn't find any passing.
        Thanks

        Show
        Ankit Agarwal added a comment - Scripts were named as indicated. Tested a few random js actions for regressions, couldn't find any passing. Thanks
        Hide
        Dan Poltawski added a comment -

        You did it!

        Thanks for your contribution, this change has been merged upstream and is now available on your local git mirror and on download sites shortly.

        Show
        Dan Poltawski added a comment - You did it! Thanks for your contribution, this change has been merged upstream and is now available on your local git mirror and on download sites shortly.

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: