Details

    • Testing Instructions:
      Hide

      Note: As always, recommend having the developer toolset open
      Note: This must be tested with developer modes ($CFG->debug) both enabled and disabled

      • Open various pages
        • Confirm that all JS resources are loaded correctly through yui_combo.php
      • When developer mode is disabled:
        • Confirm that the URL for all moodle JS loaded through yui_combo.php have -min.js on the end of the filename
        • Confirm that non-shifted files (everything but tooltip) are *not minified (there is no minified version)*
        • Confirm that tooltip *is minified*
      • When developer mode is enabled:
        • Confirm that the URL for all moodle JS loaded through yui_combo.php do not have -min.js on the end of the filename
        • Confirm that none of the JS loaded through yui_combo.php is minified
      • Navigate to a course enrolment page
      • Open your Network tab and clear the current state (to make your life easier)
      • Open the role quick add page
        • Confirm that the popup was displayed correctly
        • Confirm that a call to yui_image.php was made and returned with a 200 error code, and a valid image
        • Confirm that the [X] icon in the top-right corner was displayed correctly
      • Restrict the Network tab to JavaScript
      • Log out and navigate to the Login page
        • Confirm that moodle-core-tooltip loaded successfully in your browser
        • Confirm that a minified version was loaded if debugging was not enabled
        • Confirm that a non-minified version was loaded if debugging was enabled
        • Confirm that clicking on the [?] icon brings up the help popup correctly
      Show
      Note: As always, recommend having the developer toolset open Note: This must be tested with developer modes ($CFG->debug) both enabled and disabled Open various pages Confirm that all JS resources are loaded correctly through yui_combo.php When developer mode is disabled: Confirm that the URL for all moodle JS loaded through yui_combo.php have -min.js on the end of the filename Confirm that non-shifted files (everything but tooltip) are *not minified (there is no minified version)* Confirm that tooltip *is minified* When developer mode is enabled : Confirm that the URL for all moodle JS loaded through yui_combo.php do not have -min.js on the end of the filename Confirm that none of the JS loaded through yui_combo.php is minified Navigate to a course enrolment page Open your Network tab and clear the current state (to make your life easier) Open the role quick add page Confirm that the popup was displayed correctly Confirm that a call to yui_image.php was made and returned with a 200 error code, and a valid image Confirm that the [X] icon in the top-right corner was displayed correctly Restrict the Network tab to JavaScript Log out and navigate to the Login page Confirm that moodle-core-tooltip loaded successfully in your browser Confirm that a minified version was loaded if debugging was not enabled Confirm that a non-minified version was loaded if debugging was enabled Confirm that clicking on the [?] icon brings up the help popup correctly
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
    • Rank:
      46695

      Description

      As raised in discussion on the General Developer Forum at https://moodle.org/mod/forum/discuss.php?d=217450, shifter has some great benefits that I feel we should explore.

      In summary these benefits include:

      • minification of JS;
      • metadata creation which we can use for dependency management to inform loader configuration and reduce page waterfalls;
      • linting of JS code using the same tools as the YUI team use themselves;
      • the ability to run JS tests and code coverage; and
      • the ability to leave various logging in place within debug versions of code to aid debugging.

      Of course, all of this is not free and it does require that developers install the shifter tool on their machines. This itself requires [node.js](http://nodejs.org), though nodejs can be installed using [composer](https://packagist.org/packages/nervo/node).

      Also, unfortunately the change prevents direct use of the source JS files without them going through some form of build process. This can be achieved easily with shifter which has a --watch function to build files as they change. Additionally, it is possible to perform the basic build process within the YUI loader to allow users to make some basic changes if they wish (but only in jsrev = -1 mode).

      Such a change would not be mandated for non-core modules, but I believe that we should try and get all core modules to move to the new system ASAP. The major benefit here is Linting which would likely improve our computability with some older browsers. Through changes to the YUI combo loader, it's possible to support both methods with the new YUI module method being used in preference.

      We would of course need to update our documentation to support developers, though this is something that we really need to do for JS anyway.

        Issue Links

          Activity

          Hide
          Andrew Nicols added a comment -

          Adding all those who commented on this issue as watchers (sorry guys )

          Show
          Andrew Nicols added a comment - Adding all those who commented on this issue as watchers (sorry guys )
          Hide
          Petr Škoda added a comment -

          Adding myself too, you have my support.

          Show
          Petr Škoda added a comment - Adding myself too, you have my support.
          Hide
          Dan Poltawski added a comment -

          This seems like a very process specific thing more than a code things. I'd be interested to see a document which briefly outlines what the process is for each role in this.

          e.g.

          • What does a developer submitting a patch need to do?
          • What does the integration team need to do?
          • Do we accept patches in core which haven't run thought he shifter install? If so, what is the process for then getting shifter generated code?
          • How do we avoid mismatches between preprocessed files and source files?

          These questions seem more important to me right now than the code.

          Personally I have no problem with running something like this, but we get feedback that the barrier to entry of writing core code is harder and harder. Theoretically, it could be part of the integration process to generate the post-processed files.

          Show
          Dan Poltawski added a comment - This seems like a very process specific thing more than a code things. I'd be interested to see a document which briefly outlines what the process is for each role in this. e.g. What does a developer submitting a patch need to do? What does the integration team need to do? Do we accept patches in core which haven't run thought he shifter install? If so, what is the process for then getting shifter generated code? How do we avoid mismatches between preprocessed files and source files? These questions seem more important to me right now than the code. Personally I have no problem with running something like this, but we get feedback that the barrier to entry of writing core code is harder and harder. Theoretically, it could be part of the integration process to generate the post-processed files.
          Hide
          Tim Hunt added a comment -

          (Numbered lists make replying so much easier. Assume the bullets are 1. - 4.

          1. Either have run Shifter, or not, at their choice.

          2. When the patch developer has not run Shifter, then run it for them. See below.

          3. Yes. Integrators need to run Shifter. See below.

          4. The weekly build script much re-run Shifter on everything early in the weekly build process.

          I can see several ways in which Integrators might run Shifter, and the basic choice is to either run it for each individual commit that is integrated, or once per weekly build.

          a. Integrators will have Shifter running anyway (since they are developers). After reviewing the non-minified JS, then will need to see if their copy of Shifter has created different minified JS. If so, they will have to commit those changes.

          (Note that this is important. This prevents developer submitting a patch with innocent non-minified JS, and malicious code hidden in the minified JS.)

          b. Like a., but amend the original comment, rather than creating a new commit.

          c. Rather than committing minified JS updates separately for every comment, instead we just to one big commit (if necessary) per weekly build, which re-builds any minified JS that needs it. This means that you can only test code from integration.git if you have Shifter installed yourself.

          Note that c. is necessary anyway, for example when Shifter itself is improved, there may be changes to the minified files, even if there is no change in the Moodle source. So, like installer lang files, we need to check that everything is up-to-date before releasing each weekly.

          Show
          Tim Hunt added a comment - (Numbered lists make replying so much easier. Assume the bullets are 1. - 4. 1. Either have run Shifter, or not, at their choice. 2. When the patch developer has not run Shifter, then run it for them. See below. 3. Yes. Integrators need to run Shifter. See below. 4. The weekly build script much re-run Shifter on everything early in the weekly build process. I can see several ways in which Integrators might run Shifter, and the basic choice is to either run it for each individual commit that is integrated, or once per weekly build. a. Integrators will have Shifter running anyway (since they are developers). After reviewing the non-minified JS, then will need to see if their copy of Shifter has created different minified JS. If so, they will have to commit those changes. (Note that this is important. This prevents developer submitting a patch with innocent non-minified JS, and malicious code hidden in the minified JS.) b. Like a., but amend the original comment, rather than creating a new commit. c. Rather than committing minified JS updates separately for every comment, instead we just to one big commit (if necessary) per weekly build, which re-builds any minified JS that needs it. This means that you can only test code from integration.git if you have Shifter installed yourself. Note that c. is necessary anyway, for example when Shifter itself is improved, there may be changes to the minified files, even if there is no change in the Moodle source. So, like installer lang files, we need to check that everything is up-to-date before releasing each weekly.
          Hide
          Dan Poltawski added a comment -

          [Adding all the integrators and MD here].

          If this becomes an 'integration process', that certainly sounds achievable. Although why would a developer bother running shifter themselves?

          Show
          Dan Poltawski added a comment - [Adding all the integrators and MD here] . If this becomes an 'integration process', that certainly sounds achievable. Although why would a developer bother running shifter themselves?
          Hide
          Dan Poltawski added a comment -

          Urm, could we get conflicts in the generated minified JS? That could be ugly.

          Show
          Dan Poltawski added a comment - Urm, could we get conflicts in the generated minified JS? That could be ugly.
          Hide
          Tim Hunt added a comment -

          In the case of merge conflicts, you would just let Shifter re-build the minified file from the correctly merged source, and git add that. No worries.

          That is just one of the reasons that developers would want to run Shifter themselves. The other main reason is to test something closer to what will be used in production while doing development.

          Show
          Tim Hunt added a comment - In the case of merge conflicts, you would just let Shifter re-build the minified file from the correctly merged source, and git add that. No worries. That is just one of the reasons that developers would want to run Shifter themselves. The other main reason is to test something closer to what will be used in production while doing development.
          Hide
          Damyon Wiese added a comment -

          The listed benefits of shifter are:
          Minification,
          Removal of Y.log,
          Linting,
          Dependency metadata,
          Testing (I don't know how this helps)

          Some downsides I can see are:
          Barriers to new developers,
          Conflicts in the minified JS + generated files should not be in git (IMO).

          An alternative to shifter could be just to add the dependencies for jslint to composer and add a unit test to run each module through jslint. And fix the current minification (including stripping Y.log).

          So - I'm sitting on the fence on this.

          Show
          Damyon Wiese added a comment - The listed benefits of shifter are: Minification, Removal of Y.log, Linting, Dependency metadata, Testing (I don't know how this helps) Some downsides I can see are: Barriers to new developers, Conflicts in the minified JS + generated files should not be in git (IMO). An alternative to shifter could be just to add the dependencies for jslint to composer and add a unit test to run each module through jslint. And fix the current minification (including stripping Y.log). So - I'm sitting on the fence on this.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I don't understand a word. Will try to improve that. LOL.

          Show
          Eloy Lafuente (stronk7) added a comment - I don't understand a word. Will try to improve that. LOL.
          Hide
          Davo Smith added a comment -

          Damyon - if the minified JS is not included in Git, then it is going to be difficult for anyone who maintains their Moodle install via Git (but who is not a developer).

          Show
          Davo Smith added a comment - Damyon - if the minified JS is not included in Git, then it is going to be difficult for anyone who maintains their Moodle install via Git (but who is not a developer).
          Hide
          Damyon Wiese added a comment - - edited

          Agreed - we can't do that (not put the minified files in git) unless we add shifter and all it's dependencies to Moodle for everyone (which we shouldn't). I'm just pointing out that it's a downside to using shifter in particular over the current php based javascript minifier.

          Show
          Damyon Wiese added a comment - - edited Agreed - we can't do that (not put the minified files in git) unless we add shifter and all it's dependencies to Moodle for everyone (which we shouldn't). I'm just pointing out that it's a downside to using shifter in particular over the current php based javascript minifier.
          Hide
          Andrew Nicols added a comment -

          I'll try and cover as many of the points as I can in one go...

          Dan's comments

          What does a developer submitting a patch need to do?

          If the developer is just working on a specific YUI module or set of modules in a part of moodle (e.g. course, lib, mod/xxx, block/xxx), then the easiest thing to do is to have a second terminal cd'd into the relevant yui/src directory and running shifter --watch. This will pick up any change that the developer is working on within the module and shift it (minify, lint, test, etc).

          What does the integration team need to do?

          Technically they shouldn't need to do anything, but they should probably run shifter --walk --recursive in the TLD when cherry-picking. Of course, there should be no changes, and ideally there should be no lint issues occurring.

          Longer term, when we get tests (JS Unit tests courtesy of http://yuilibrary.com/yui/docs/test/) and code coverage review of those tests (courtesy of istanbul http://gotwarlost.github.com/istanbul/), then we will probably want to hook shifter in to the jenkins phase too.

          Do we accept patches in core which haven't run thought he shifter install? If so, what is the process for then getting shifter generated code?

          That's one of the things I was hoping to raise as an area for discussion both in this issue and the original forum thread. I think that the options are really.

          If we do add nodejs to the composer requirements then it's really easy to install. Unfortunately the shifter installation is currently global, but I believe it should be possible to make a non-global install of it (global installations require root, non-global are user-specific). Install shifter through node is really really easy:

          npm -g install shifter
          

          Given this, I would hope that anyone on the list of potential assignees would have composer installed and thus have node.js and be in a position to install shifter too (I realise that this is a lot to ask or mandate though).

          I think that the options are therefore:

          1. Require the person submitting the patch to run shifter and reject the patch without. This the worst option IMO. It leads to poor inclusion and makes it harder for people to get involved.
          2. Any non-shifted change would need to be picked up by a sponsor (anyone on the assignees list, but most likely the component lead) who would then take the patch on. This is possibly the best option in many respects.
          3. Any non-shifted change would be shifted by the integrator. This is an okay option, especially since the integrators would probably already be running shifter at cherry-pick time; but it does lead to potential unnecessary noise in the repo.

          How do we avoid mismatches between preprocessed files and source files?

          Ideally, we look at installing a specific version of shifter and it's dependencies using composer.
          I don't think that we'll see changes in the minified versions too frequently on the whole anyway, but periodically the required shifter version would be updated and a build would take place of the whole repo:

          cd git/moodle
          shifter --walk --recursive
          

          This would avoid a majority of mismatches and reduce the impact of them.

          Personally I have no problem with running something like this, but we get feedback that the barrier to entry of writing core code is harder and harder. Theoretically, it could be part of the integration process to generate the post-processed files.

          It could quite easily be part of the integration process. The only potential issue with this is that to use shifter, we have to remove the boilerplate:

          YUI.add('moodle-modtype-modname', function(Y, NAME) {
          ...
          }, {"requires": ["foo-bar", "moodle-core-baz"]});
          

          And we're just left with the rest of the code. I have put a wrapper to add this (thanks for the suggestion Tim) so this isn't a major major issue really. This wrapper (which is only applied when $CFG->jsrev = 1;and $CFG>jsuseshifter === true) does have some potential caveats and may require further development to get everything in place (it currently doesn't support rollups which may be really helpful to us in the future).

          Additionally, in some ways it actually removes a barrier. The above boiler plate looks scary if you don't know what it's for and isn't massively obvious. By generating this boilerplate based upon the dependency meta-data and the module configuration (which are easily templateable and fairly clear) it may make life easier for people wanting to write a quick YUI module and not knowing where to start.

          Damyon's comments:

          Barriers to new developers

          As I say, this is the one that concerns me most. If we're advising people to run composer, and we can install shifter through composer then this shouldn't be a major concern and I've covered this under Dan's comments.

          Conflicts in the minified JS + generated files should not be in git (IMO).

          We shouldn't really see conflicts and I've addresse this above in my responses to Dan's comments.

          An alternative to shifter could be just to add the dependencies for jslint to composer and add a unit test to run each module through jslint. And fix the current minification (including stripping Y.log).

          We could add the dependencies for jslint to composer, but which jslint to we use? Are we looking at the node.js version or another? Is it a wise idea to use different tools for our code than the rest of YUI? Will we pick up all of the same issues?
          The same goes for the minification options. As Amy pointed out on the forum thread, minifiers are fussy and we don't match our linter to our minifier, we could end up with poorly minified code (which would be bad).

          Addendum

          One of the other points that Petr made (in the forum thread) was that it would be good to drop support for a lot of the other JS inclusion methods. This could also really aid us here whilst still allowing people to use non-YUI code (e.g. jquery, or native). Additionally, they'd still benefitting from all of the features of shifter. Furthermore, we could more safely drop all of the other JS inclusion methods (e.g. /javascript.php)

          Show
          Andrew Nicols added a comment - I'll try and cover as many of the points as I can in one go... Dan's comments What does a developer submitting a patch need to do? If the developer is just working on a specific YUI module or set of modules in a part of moodle (e.g. course, lib, mod/xxx, block/xxx), then the easiest thing to do is to have a second terminal cd'd into the relevant yui/src directory and running shifter --watch. This will pick up any change that the developer is working on within the module and shift it (minify, lint, test, etc). What does the integration team need to do? Technically they shouldn't need to do anything, but they should probably run shifter --walk --recursive in the TLD when cherry-picking. Of course, there should be no changes, and ideally there should be no lint issues occurring. Longer term, when we get tests (JS Unit tests courtesy of http://yuilibrary.com/yui/docs/test/ ) and code coverage review of those tests (courtesy of istanbul http://gotwarlost.github.com/istanbul/ ), then we will probably want to hook shifter in to the jenkins phase too. Do we accept patches in core which haven't run thought he shifter install? If so, what is the process for then getting shifter generated code? That's one of the things I was hoping to raise as an area for discussion both in this issue and the original forum thread. I think that the options are really. If we do add nodejs to the composer requirements then it's really easy to install. Unfortunately the shifter installation is currently global, but I believe it should be possible to make a non-global install of it (global installations require root, non-global are user-specific). Install shifter through node is really really easy: npm -g install shifter Given this, I would hope that anyone on the list of potential assignees would have composer installed and thus have node.js and be in a position to install shifter too (I realise that this is a lot to ask or mandate though). I think that the options are therefore: Require the person submitting the patch to run shifter and reject the patch without. This the worst option IMO. It leads to poor inclusion and makes it harder for people to get involved. Any non-shifted change would need to be picked up by a sponsor (anyone on the assignees list, but most likely the component lead) who would then take the patch on. This is possibly the best option in many respects. Any non-shifted change would be shifted by the integrator. This is an okay option, especially since the integrators would probably already be running shifter at cherry-pick time; but it does lead to potential unnecessary noise in the repo. How do we avoid mismatches between preprocessed files and source files? Ideally, we look at installing a specific version of shifter and it's dependencies using composer. I don't think that we'll see changes in the minified versions too frequently on the whole anyway, but periodically the required shifter version would be updated and a build would take place of the whole repo: cd git/moodle shifter --walk --recursive This would avoid a majority of mismatches and reduce the impact of them. Personally I have no problem with running something like this, but we get feedback that the barrier to entry of writing core code is harder and harder. Theoretically, it could be part of the integration process to generate the post-processed files. It could quite easily be part of the integration process. The only potential issue with this is that to use shifter, we have to remove the boilerplate: YUI.add('moodle-modtype-modname', function(Y, NAME) { ... }, { "requires" : [ "foo-bar" , "moodle-core-baz" ]}); And we're just left with the rest of the code. I have put a wrapper to add this (thanks for the suggestion Tim) so this isn't a major major issue really. This wrapper (which is only applied when $CFG->jsrev = 1;and $CFG >jsuseshifter === true) does have some potential caveats and may require further development to get everything in place (it currently doesn't support rollups which may be really helpful to us in the future). Additionally, in some ways it actually removes a barrier. The above boiler plate looks scary if you don't know what it's for and isn't massively obvious. By generating this boilerplate based upon the dependency meta-data and the module configuration (which are easily templateable and fairly clear) it may make life easier for people wanting to write a quick YUI module and not knowing where to start. Damyon's comments: Barriers to new developers As I say, this is the one that concerns me most. If we're advising people to run composer, and we can install shifter through composer then this shouldn't be a major concern and I've covered this under Dan's comments. Conflicts in the minified JS + generated files should not be in git (IMO). We shouldn't really see conflicts and I've addresse this above in my responses to Dan's comments. An alternative to shifter could be just to add the dependencies for jslint to composer and add a unit test to run each module through jslint. And fix the current minification (including stripping Y.log). We could add the dependencies for jslint to composer, but which jslint to we use? Are we looking at the node.js version or another? Is it a wise idea to use different tools for our code than the rest of YUI? Will we pick up all of the same issues? The same goes for the minification options. As Amy pointed out on the forum thread, minifiers are fussy and we don't match our linter to our minifier, we could end up with poorly minified code (which would be bad). Addendum One of the other points that Petr made (in the forum thread) was that it would be good to drop support for a lot of the other JS inclusion methods. This could also really aid us here whilst still allowing people to use non-YUI code (e.g. jquery, or native). Additionally, they'd still benefitting from all of the features of shifter. Furthermore, we could more safely drop all of the other JS inclusion methods (e.g. /javascript.php)
          Hide
          Andrew Nicols added a comment -

          Has anyone else got any thoughts on this suggestion?

          Show
          Andrew Nicols added a comment - Has anyone else got any thoughts on this suggestion?
          Hide
          Andrew Nicols added a comment -

          One of the benefits of minification would be that we could actually improve the documentation for our content.

          Over the christmas break, I had a play with YUIDoc to generate some documentation for our Moodle-specific JS modules. See http://sandpit.andrewrn.co.uk/moodle/ for an example of that built documentation.

          At present, I'm rather loathe to do suggest that we start merrily writing documentation though because every line of documentation is still present on the delivered version. The more documentation we add, the more documentation we ship to end users and the larger our JS files get.

          If we can move to shifter, then improving our documentation and running it through shifter would be a very natural next step.

          Show
          Andrew Nicols added a comment - One of the benefits of minification would be that we could actually improve the documentation for our content. Over the christmas break, I had a play with YUIDoc to generate some documentation for our Moodle-specific JS modules. See http://sandpit.andrewrn.co.uk/moodle/ for an example of that built documentation. At present, I'm rather loathe to do suggest that we start merrily writing documentation though because every line of documentation is still present on the delivered version. The more documentation we add, the more documentation we ship to end users and the larger our JS files get. If we can move to shifter, then improving our documentation and running it through shifter would be a very natural next step.
          Hide
          Amy Groshek added a comment -

          Just to add another perspective regarding barriers to entry, there are a lot of new Web development tools out there: Jade for HTML; Grunt for JavaScript; Sass, Compass, and Less for CSS; and the plethora of JS unit-testing tools. They all require installations and configurations just to start your basic workflow. This is clearly where the industry is headed. (My guess is that at some point there will be a corresponding request that we adapt our theme development workflow to allow for a CSS preprocessor and csshint. And at that time, it's likely that some of the most popular, well-supported tools available will be running on node.)

          Show
          Amy Groshek added a comment - Just to add another perspective regarding barriers to entry, there are a lot of new Web development tools out there: Jade for HTML; Grunt for JavaScript; Sass, Compass, and Less for CSS; and the plethora of JS unit-testing tools. They all require installations and configurations just to start your basic workflow. This is clearly where the industry is headed. (My guess is that at some point there will be a corresponding request that we adapt our theme development workflow to allow for a CSS preprocessor and csshint. And at that time, it's likely that some of the most popular, well-supported tools available will be running on node.)
          Hide
          Martin Dougiamas added a comment -

          My take on it (with not deep research) is that the tradeoff of minification, linting and testing sounds worth the small barrier for those developers writing JS modules. If I understand correctly other developers are not affected.

          So I give my +1 on condition that the integration team unanimously accepts the extra load.

          Show
          Martin Dougiamas added a comment - My take on it (with not deep research) is that the tradeoff of minification, linting and testing sounds worth the small barrier for those developers writing JS modules. If I understand correctly other developers are not affected. So I give my +1 on condition that the integration team unanimously accepts the extra load.
          Hide
          Andrew Nicols added a comment -

          This is the initial work for shifter.
          I have also included a shifted version of moodle-core-tooltip to help with testing. I understand if you'd rather have this handled in a separate issue.

          Show
          Andrew Nicols added a comment - This is the initial work for shifter. I have also included a shifted version of moodle-core-tooltip to help with testing. I understand if you'd rather have this handled in a separate issue.
          Hide
          Andrew Nicols added a comment -

          Putting this up for peer review so we can get the ball rolling. We still need to finalise details of how this will affect integration workflow.

          The changes are fairly minimal. Essentially we try and loaded a shifted version of modules and supporting assets from the build/frankenstyle-name directory first and then fall back to the existing locations upon failure.

          Converting our existing modules to a shifted version should be addressed in a separate issue.

          We won't reap the benefits of the separated meta-data immediately, but in due course we should be able to look at grabbing this all together with MUC. Once this issue lands, I'll open an appropriate issue for that too.

          Show
          Andrew Nicols added a comment - Putting this up for peer review so we can get the ball rolling. We still need to finalise details of how this will affect integration workflow. The changes are fairly minimal. Essentially we try and loaded a shifted version of modules and supporting assets from the build/frankenstyle-name directory first and then fall back to the existing locations upon failure. Converting our existing modules to a shifted version should be addressed in a separate issue. We won't reap the benefits of the separated meta-data immediately, but in due course we should be able to look at grabbing this all together with MUC. Once this issue lands, I'll open an appropriate issue for that too.
          Hide
          Tim Hunt added a comment -

          +1 from me

          [Y] Syntax
          [-] Output
          [Y] Whitespace
          [-] Language
          [-] Databases
          [Y] Testing
          [Y] Security
          [Y] Documentation
          [Y] Git
          [Y] Sanity check

          Show
          Tim Hunt added a comment - +1 from me [Y] Syntax [-] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing [Y] Security [Y] Documentation [Y] Git [Y] Sanity check
          Hide
          Andrew Nicols added a comment -

          Thanks Tim,

          Do you think this should go through additional peer review (or have you just not followed the workflow through)?

          Changes to developer/peer reviewer/integrator workflows

          With regards to change of workflow for different users, my proposal is that developers build their changes. For the most part this should be fine. We should verify those changes as part of our automated testing though.
          There maybe times when Integrator may need to run a build too.

          Developers

          Developers should submit both their shifted and source files.
          This may lead to conflicts in the built files if two issues address the same source file in the same integration week. I think that where a developer is aware that their changes may conflict with others in that weeks' integration, they should still check those files in, but warn integrators that other changes will cause a conflict in the built files and that the integrator will need to shift that module.

          Alternatively, developers may choose to base one of their commits upon the other and build both times warning integrators as usual.

          Peer Reviewer

          I don't think that Peer Reviewers need adjust their workflow. If they're being particularly thorough, then they may choose to confirm the files when they confirm the changes.

          Integrator

          Integrators should not have to change their workflow particularly, but may find that they need to run the occasional `shifter --walk --recursive` when multiple changes have been made to the same source file.

          Jenkins

          Jenkins should run a new job for `shifter --walk --recursive`. It shouldn't expect any changes. If there are changes, then it should fail the test and we should either:

          • send back as testing failed as per normal procedures; or
          • just build the change and commit it (which is probably what the other option would end up with anyway).

          Andrew

          Show
          Andrew Nicols added a comment - Thanks Tim, Do you think this should go through additional peer review (or have you just not followed the workflow through)? Changes to developer/peer reviewer/integrator workflows With regards to change of workflow for different users, my proposal is that developers build their changes. For the most part this should be fine. We should verify those changes as part of our automated testing though. There maybe times when Integrator may need to run a build too. Developers Developers should submit both their shifted and source files. This may lead to conflicts in the built files if two issues address the same source file in the same integration week. I think that where a developer is aware that their changes may conflict with others in that weeks' integration, they should still check those files in, but warn integrators that other changes will cause a conflict in the built files and that the integrator will need to shift that module. Alternatively, developers may choose to base one of their commits upon the other and build both times warning integrators as usual. Peer Reviewer I don't think that Peer Reviewers need adjust their workflow. If they're being particularly thorough, then they may choose to confirm the files when they confirm the changes. Integrator Integrators should not have to change their workflow particularly, but may find that they need to run the occasional `shifter --walk --recursive` when multiple changes have been made to the same source file. Jenkins Jenkins should run a new job for `shifter --walk --recursive`. It shouldn't expect any changes. If there are changes, then it should fail the test and we should either: send back as testing failed as per normal procedures; or just build the change and commit it (which is probably what the other option would end up with anyway). Andrew
          Hide
          Tim Hunt added a comment -

          Sorry, I just forgot to click the buttons. I think your proposed workflows are good too.

          Show
          Tim Hunt added a comment - Sorry, I just forgot to click the buttons. I think your proposed workflows are good too.
          Hide
          Andrew Nicols added a comment -

          Thanks Tim,

          Submitting for integration.

          Show
          Andrew Nicols added a comment - Thanks Tim, Submitting for integration.
          Hide
          Amy Groshek added a comment -

          Anything I can do to help out with this?

          Show
          Amy Groshek added a comment - Anything I can do to help out with this?
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          +0.5 (thanks to Sam for his explanations). The only point that I need verification about (from an integration POV) is:

          does "shifter --walk --recursive" produce constant results? I mean, if i run it here, by commit, or manually before pushing... will we get 100% the same "shifter-ed" files (aka, no diff) ? (if the source has not changed, obviously, heh)

          Note only acceptable answer is "yes, 100%".

          Show
          Eloy Lafuente (stronk7) added a comment - - edited +0.5 (thanks to Sam for his explanations). The only point that I need verification about (from an integration POV) is: does "shifter --walk --recursive" produce constant results? I mean, if i run it here, by commit, or manually before pushing... will we get 100% the same "shifter-ed" files (aka, no diff) ? (if the source has not changed, obviously, heh) Note only acceptable answer is "yes, 100%".
          Hide
          Andrew Nicols added a comment -

          The answer is yes, 99.9%
          The 0.1% comes from potential scenario where someone is running a different version of one of the libraries doing the hard work - specifically yuglify (the JS and CSS minifier).
          In reality this rarely happens. Shifter's package.json has versioned dependencies on yuglify and ycssmin and these are rarely updated.

          To avoid this, we could build and push our own npm package for moodlejs development. Here's one I made earlier: https://github.com/andrewnicols/moodlejs

          It installs specific versions of shifter, yuidoc, and jshint.
          Because npm (the node.js package manager) is designed to allow your package to have dependencies at a different version to other packages, I've had to namespace the binaries:

          • mshifter
          • myuidoc
          • mjshint

          This sucks a bit, but I don't think there's a way of saying "I want to globally install shifter at this version" (see https://npmjs.org/doc/faq.html#Why-can-t-npm-just-put-everything-in-one-place-like-other-package-managers and https://npmjs.org/doc/faq.html#Whatever-I-really-want-the-old-style-everything-global-style).

          If you want to try this:

          npm install https://github.com/andrewnicols/moodlejs/tarball/master -g
          

          You can then run:

          mshifter --walk --recursive
          mjshint lib/yui/tooltip/tooltip.js
          myuidoc
          

          Unfortunately, this may mean that some IDEs don't pick up jshint without a symlink linking mjshint to jshint

          If we like this as a solution, I'll publish it to npm so that people can just run `sudo npm install -g` and we can manage it centrally.

          Show
          Andrew Nicols added a comment - The answer is yes, 99.9% The 0.1% comes from potential scenario where someone is running a different version of one of the libraries doing the hard work - specifically yuglify (the JS and CSS minifier). In reality this rarely happens. Shifter's package.json has versioned dependencies on yuglify and ycssmin and these are rarely updated. To avoid this, we could build and push our own npm package for moodlejs development. Here's one I made earlier: https://github.com/andrewnicols/moodlejs It installs specific versions of shifter, yuidoc, and jshint. Because npm (the node.js package manager) is designed to allow your package to have dependencies at a different version to other packages, I've had to namespace the binaries: mshifter myuidoc mjshint This sucks a bit, but I don't think there's a way of saying "I want to globally install shifter at this version" (see https://npmjs.org/doc/faq.html#Why-can-t-npm-just-put-everything-in-one-place-like-other-package-managers and https://npmjs.org/doc/faq.html#Whatever-I-really-want-the-old-style-everything-global-style ). If you want to try this: npm install https: //github.com/andrewnicols/moodlejs/tarball/master -g You can then run: mshifter --walk --recursive mjshint lib/yui/tooltip/tooltip.js myuidoc Unfortunately, this may mean that some IDEs don't pick up jshint without a symlink linking mjshint to jshint If we like this as a solution, I'll publish it to npm so that people can just run `sudo npm install -g` and we can manage it centrally.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I understand nothing, lol.

          But for sure we must have an "official" version of the build tool to be used (that way we can detect discrepancy in built files).

          That or we ignore the shifter-ed built files from developers and always re-shifter before pushing (or an automated process does that).

          Those are the 2 alternatives I see. Perhaps the 1st is a bit horrible for developers. But the second implies that we simply will be ignoring built files coming from developers and always will be rebuilding them as part of the integration process.

          Feel free, I'm happy with any. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I understand nothing, lol. But for sure we must have an "official" version of the build tool to be used (that way we can detect discrepancy in built files). That or we ignore the shifter-ed built files from developers and always re-shifter before pushing (or an automated process does that). Those are the 2 alternatives I see. Perhaps the 1st is a bit horrible for developers. But the second implies that we simply will be ignoring built files coming from developers and always will be rebuilding them as part of the integration process. Feel free, I'm happy with any. Ciao
          Hide
          Sam Hemelryk added a comment -

          Ok, what I really like so far is that everyone is in agreement that this should land.
          We've really just got to decide how to best handle the building and integration processes.

          I'm going to integrate this now and we can continue the discussion with confidence and urgency that it has landed.

          In the words of Eloy the Wondernificent "let's break it, we have 1 month to go back "

          I'll lay out what I think the plan should be in a minute.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Ok, what I really like so far is that everyone is in agreement that this should land. We've really just got to decide how to best handle the building and integration processes. I'm going to integrate this now and we can continue the discussion with confidence and urgency that it has landed. In the words of Eloy the Wondernificent "let's break it, we have 1 month to go back " I'll lay out what I think the plan should be in a minute. Many thanks Sam
          Hide
          Sam Hemelryk added a comment -

          Integrated!

          Show
          Sam Hemelryk added a comment - Integrated!
          Hide
          Sam Hemelryk added a comment -

          So as how we should treat things and handle the build process. To me Andrews process purposed above makes the most sense.
          Just to summarise how I've read it:

          • Decide upon a version of each required tool and use that internally (integrators + CI server) to ensure that our build processes are going to be identical.
          • Make a recommendation that developers use the "offical" versions of these tools. Don't enforce this as a requirement its just a recommendation.
          • Require that developers submit built files as part of a change. Two reasons: a -they've tested their changes (you've got to build to test) b - we'll be able to pick up upon discrepancies between build processes and perhaps have some indication about upcoming issues or issues that have been fixed in later versions.
          • During peer-review and integration review check that build files have been updated.
          • When an issue is integrated have CI bot run the build process. If the outcome results in changes have it complain and if possible make a commit to fix the build files.

          Essentially the basis of this plan is that we always build everything ourselves anyway. As such you could ignore the build files from contribution however I think that by accepting them we benefit (the two benefits noted above) and that we avoid needless commits as we'd only commit on discrepancies.

          What does everyone else think?

          Show
          Sam Hemelryk added a comment - So as how we should treat things and handle the build process. To me Andrews process purposed above makes the most sense. Just to summarise how I've read it: Decide upon a version of each required tool and use that internally (integrators + CI server) to ensure that our build processes are going to be identical. Make a recommendation that developers use the "offical" versions of these tools. Don't enforce this as a requirement its just a recommendation. Require that developers submit built files as part of a change. Two reasons: a -they've tested their changes (you've got to build to test) b - we'll be able to pick up upon discrepancies between build processes and perhaps have some indication about upcoming issues or issues that have been fixed in later versions. During peer-review and integration review check that build files have been updated. When an issue is integrated have CI bot run the build process. If the outcome results in changes have it complain and if possible make a commit to fix the build files. Essentially the basis of this plan is that we always build everything ourselves anyway. As such you could ignore the build files from contribution however I think that by accepting them we benefit (the two benefits noted above) and that we avoid needless commits as we'd only commit on discrepancies. What does everyone else think?
          Hide
          Tim Hunt added a comment -

          +1 to Sam's proposal.

          Show
          Tim Hunt added a comment - +1 to Sam's proposal.
          Hide
          Andrew Nicols added a comment -

          +1 from me too. FYI, you can install a specific version of a node tool with:

          npm install shifter@0.2.25 -g
          
          Show
          Andrew Nicols added a comment - +1 from me too. FYI, you can install a specific version of a node tool with: npm install shifter@0.2.25 -g
          Hide
          Eloy Lafuente (stronk7) added a comment -

          +1 sounds ok.

          Show
          Eloy Lafuente (stronk7) added a comment - +1 sounds ok.
          Hide
          Aparup Banerjee added a comment -

          +1 to the plan.

          Barriers: The documentation really needs to be built up much more though before we "Require that developers submit built files as part of a change."

          ps: I've added a doc link.

          Show
          Aparup Banerjee added a comment - +1 to the plan. Barriers: The documentation really needs to be built up much more though before we "Require that developers submit built files as part of a change." ps: I've added a doc link.
          Hide
          Michael de Raadt added a comment -

          Test result: I think it's working, but I wasn't sure exactly how to check many of the things you were asking for. I'll refrain from passing/failing this and wait for Andrew's feedback.

          I'll go through the testing instructions....

          Note: As always, recommend having the developer toolset open
          I used Firebug, mostly looking at the Net tab.

          Note: This must be tested with developer modes both enabled and disabled
          I assume you mean debugging on and off. Or do you mean something else?

          Open various pages
          I did that, varying different combinations of debugging, enablecssoptimiser, and yuicomboloading. Nothing exploded in my face.

          Confirm that all JS resources are loaded correctly through yui_combo.php
          I did see yui_combo.php coming up, but I'm not sure how to confirm that ALL JS was going through that script. I did see javascript-static.js and dock.js coming though as normal files.

          Navigate to a course enrolment page
          Confirm that the CSS and images loaded through yui_combo.php and yui_image.php loaded correctly
          I used the enrolment page with various settings on and off. I did see evidence of yui_combo.php being called, but I didn't see any sign of yui_image.php.

          Add the second commit converting moodle-core-tooltip to using shifter
          Sam confirmed this was integrated.

          Force a page refresh
          Done.

          Confirm that the tooltip loaded successfully in your browser
          By tooltip I assume you mean the help modals. They did appear.

          Confirm that a minified version was loaded if debugging was not enabled
          Yes, it was minified with debugging off.

          Confirm that a non-minified version was loaded if debugging was enabled
          Yes, it was not minified with debugging on.

          Confirm that everything works as expected
          The enrolment page worked as before.

          Show
          Michael de Raadt added a comment - Test result: I think it's working, but I wasn't sure exactly how to check many of the things you were asking for. I'll refrain from passing/failing this and wait for Andrew's feedback. I'll go through the testing instructions.... Note: As always, recommend having the developer toolset open I used Firebug, mostly looking at the Net tab. Note: This must be tested with developer modes both enabled and disabled I assume you mean debugging on and off. Or do you mean something else? Open various pages I did that, varying different combinations of debugging, enablecssoptimiser, and yuicomboloading. Nothing exploded in my face. Confirm that all JS resources are loaded correctly through yui_combo.php I did see yui_combo.php coming up, but I'm not sure how to confirm that ALL JS was going through that script. I did see javascript-static.js and dock.js coming though as normal files. Navigate to a course enrolment page Confirm that the CSS and images loaded through yui_combo.php and yui_image.php loaded correctly I used the enrolment page with various settings on and off. I did see evidence of yui_combo.php being called, but I didn't see any sign of yui_image.php. Add the second commit converting moodle-core-tooltip to using shifter Sam confirmed this was integrated. Force a page refresh Done. Confirm that the tooltip loaded successfully in your browser By tooltip I assume you mean the help modals. They did appear. Confirm that a minified version was loaded if debugging was not enabled Yes, it was minified with debugging off. Confirm that a non-minified version was loaded if debugging was enabled Yes, it was not minified with debugging on. Confirm that everything works as expected The enrolment page worked as before.
          Hide
          Andrew Nicols added a comment -

          I've updated the testing instructions slightly to make them clearer and to add a few steps I thought of. I have just run through the testing instructions on integration/master too.

          Note: This must be tested with developer modes both enabled and disabled
          I assume you mean debugging on and off. Or do you mean something else?

          Sorry - yes this is what I mean. with debugging enabled to a high enough level for debugging() calls to print to screen, we use the un-minified version, otherwise we use the minified version. Obviously, we need to test both.

          Confirm that all JS resources are loaded correctly through yui_combo.php
          I did see yui_combo.php coming up, but I'm not sure how to confirm that ALL JS was going through that script. I did see javascript-static.js and dock.js coming though as normal files.

          Bah - I'd forgotten about those ones. Things won't change there, but all YUI scripts should go through yui_combo.php. Basically, what I was trying to say was where you see yui_combo.php, make sure there aren't any comments to say that the file couldn't be found. Given that everything works and you see the minified and non-minified versions of the tooltip, this works correctly.

          I used the enrolment page with various settings on and off. I did see evidence of yui_combo.php being called, but I didn't see any sign of yui_image.php.

          On reflection, I think that the yui_image.php calls will only appear when you open the quick enrolment popup. It's an image handler for images included by YUI module CSS (in the enrol/yui/quickenrolment/assets/skins/sam folder) so it will only fetch images when the image is requested by the CSS, which only happens when that popup is opened.

          Given your experiences, and that you haven't seen copious amounts of errors, I think that this test is probably safe to pass (especially as I've just tested everything I can think of).

          Show
          Andrew Nicols added a comment - I've updated the testing instructions slightly to make them clearer and to add a few steps I thought of. I have just run through the testing instructions on integration/master too. Note: This must be tested with developer modes both enabled and disabled I assume you mean debugging on and off. Or do you mean something else? Sorry - yes this is what I mean. with debugging enabled to a high enough level for debugging() calls to print to screen, we use the un-minified version, otherwise we use the minified version. Obviously, we need to test both. Confirm that all JS resources are loaded correctly through yui_combo.php I did see yui_combo.php coming up, but I'm not sure how to confirm that ALL JS was going through that script. I did see javascript-static.js and dock.js coming though as normal files. Bah - I'd forgotten about those ones. Things won't change there, but all YUI scripts should go through yui_combo.php. Basically, what I was trying to say was where you see yui_combo.php, make sure there aren't any comments to say that the file couldn't be found. Given that everything works and you see the minified and non-minified versions of the tooltip, this works correctly. I used the enrolment page with various settings on and off. I did see evidence of yui_combo.php being called, but I didn't see any sign of yui_image.php. On reflection, I think that the yui_image.php calls will only appear when you open the quick enrolment popup. It's an image handler for images included by YUI module CSS (in the enrol/yui/quickenrolment/assets/skins/sam folder) so it will only fetch images when the image is requested by the CSS, which only happens when that popup is opened. Given your experiences, and that you haven't seen copious amounts of errors, I think that this test is probably safe to pass (especially as I've just tested everything I can think of).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi guys, I'm passing this on behalf of Michael. Feel free to create new issues for any annoyance / problem / bug related with this shift-ing.

          Sam, perhaps your plan, that seems generally accepted could be shared in dev forum, documented in docs, and spread to new mdlsite issues for the actions to be taken? TIA!

          Thanks all!

          Show
          Eloy Lafuente (stronk7) added a comment - Hi guys, I'm passing this on behalf of Michael. Feel free to create new issues for any annoyance / problem / bug related with this shift-ing. Sam, perhaps your plan, that seems generally accepted could be shared in dev forum, documented in docs, and spread to new mdlsite issues for the actions to be taken? TIA! Thanks all!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities.

          Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied).

          Thanks, closing as fixed!

          Show
          Eloy Lafuente (stronk7) added a comment - This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities. Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied). Thanks, closing as fixed!
          Hide
          Michael de Raadt added a comment -

          Thanks for your response, Andrew.

          I look forward to seeing more YUI code minified.

          Show
          Michael de Raadt added a comment - Thanks for your response, Andrew. I look forward to seeing more YUI code minified.
          Hide
          Andrew Nicols added a comment -

          Initial set of Shifter documentation: http://docs.moodle.org/dev/Javascript/Shifter

          Show
          Andrew Nicols added a comment - Initial set of Shifter documentation: http://docs.moodle.org/dev/Javascript/Shifter

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              13 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: