General developer forum

 
 
Picture of Andrew Nicols
YUI + shifter
Group DevelopersGroup Moodle HQGroup Particularly helpful MoodlersGroup Testers

I've been pondering JS a lot since hackfest in various ways and I'm keen to get the views of other developers here. One thing which keeps striking me is that we need to improve the way that we handle moodle YUI modules. At present, we put them in place in their own folder within a yui directory and that works well for us; but we're missing a few really great things:

Historically, YUI used ant to build the project and ant is a pain to set up. However Dav Glass (lead developer on the YUI project) has written a replacement tool called shifter which is written for node.js. Thankfully, node.js is much less of a pain, much more lightweight, and shifter is much faster than the ant tools.

Shifter is great for our purposes because it handles all of the above, and it doesn't require any really major changes to our code; to use it we just need to: * adjust our directory structure a little; * move the requires data and YUI.add lines out of the module and into a meta-data file; and * define the JS module in a json file (build.json).

However, the big change is to working procedures. Because of the above changes (specifically that we move some of the metadata out of the module code) and the way that that shifter builds, it is no longer possible to just use the JS you've written without running it through shifter to add the relevant boilerplate.

Thankfully shifter makes that easy - you can run shifter using the --watch argument and it will continually look for any files it needs to build, so you can just modify the files you're working on as you develop and shifter will pick them up within a few seconds and deploy the built changes.

So in order to work on a shifted YUI module in our code, anyone wishing to develop a Moodle YUI module would have to have both node.js and shifter installed and know that you need to run them (node.js is pretty easy to install on most platforms; and to install shifter is just a case of running npm -g install shifter after install node.js).

The changes we need to make can easily be written such that it is possible to write Moodle YUI modules in the existing manner so third-party developers need not convert all of their code if they don't want to, but I believe that we should try and move all core code to this method for Moodle 2.5.

One of the biggest benefits of this change is that of added JSLint (JSHint technically). At present, almost none of the core JS we have in place passes JS Lint entirely without a single warning. Most of these are really just warnings, but some browsers really suck with some stupid things which then completely break JS for that whole page (like trailing commas on an array -- I'm looking at you IE<9). Lint would catch these early and prevent a majority of these kinds of bugs and also help improve the quality of our code in general.

One of the other benefits of using shifter is that we gain Minification of our JS for free - this doesn't sound a big deal, but some of our JS is getting pretty large and in most cases we can easily halve it's size. We would also gain the ability to add testing.

Another nice feature is that we'd also be able to use Y.log when developing JS and leave useful debugging in place. Shifter automatically removes the Y.log lines in all built files except for the -debug.js files which is perfect for those of us wanting that extra bit of information when trying to track down an issue.

My big concern here though is that this may add an additional barrier to people wanting to write YUI modules for Moodle and stop people wanting to contribute.

I'd love to hear any thoughts people have on this matter.

If you want to have a look at the kinds of changes that this would involve, I've pushed an example to github. You can build these either by:

  • cd course/yui/src ; shifter --walk
  • shifter --walk --recursive

If you choose the --recursive option, you need to be running a recent version of shifter (since yesterday).

Thanks in advance,

Andrew

 
Average of ratings:Useful (4)
Picture of Andrew Nicols
Re: YUI + shifter
Group DevelopersGroup Moodle HQGroup Particularly helpful MoodlersGroup Testers
I've just updated my branch on github to separate out all of the changes for easier inspection. There are now separate commits for:
* changes to yui_combo.php;
* each of the modules I've changed; and
* the build for each module.
 
Average of ratings: -
Tim at Lone Pine Koala Sanctuary
Re: YUI + shifter
Group DevelopersGroup Documentation writersGroup Particularly helpful Moodlers

It looks like the potential benefits are quite significant.

For people who do a lot of Moodle development, the extra set-up cost is clearly worth it for linting and tests and so on.

The scenario that worries me is an occasional Moodle developer, who finds and wants to fix a one-line bug in some existing JS. It would be nice if they could just edit the src, and test it. Basically, make something like the existing theme designer mode still work if you don't have node.js and everything installed.

Then, if someone submits a patch that just updates the /src JavaScript, the integrators would be able to generate the necessary change to the /build JavaScript files.

 
Average of ratings: -
Picture of Andrew Nicols
Re: YUI + shifter
Group DevelopersGroup Moodle HQGroup Particularly helpful MoodlersGroup Testers
Perhaps we could have this as an option somehow - for those of us who have taken the jump and installed shifter and are using it, we wouldn't want the fallback src-only mode to take precedent.

I guess this could be an additional developer setting whereby you set that you *do have* shifter enabled and take responsibility for that side of things.
 
Average of ratings: -
Dan at desk in Moodle HQ, Perth
Re: YUI + shifter
Group DevelopersGroup Moodle Course Creator Certificate holdersGroup Moodle HQGroup Particularly helpful MoodlersGroup Testers

The scenario that worries me is an occasional Moodle developer, who finds and wants to fix a one-line bug in some existing JS.

Right. Or a contrib plugin dev, or a university customisation.

I'm afraid that Moodle developer culture isn't really suited towards running build scripts like this. We could do it as a 'producing the build' step in core, but I'm not sure it really fits.

 
Average of ratings: -
Picture of Andrew Nicols
Re: YUI + shifter
Group DevelopersGroup Moodle HQGroup Particularly helpful MoodlersGroup Testers
> Right. Or a contrib plugin dev, or a university customisation.

For contrib plugins, we can still support the non-shifted alternative. I'm just proposing adding the preference to use a shifted version if it's available and that we should only force it in core.

I've nearly finished implementing Tim's suggestion of attempting to build the additional metadata into the source file on the fly for people with theme designer mode (well, I went for jsrev = -1 rather than theme designer mode) which would allow people to fiddle with the JS and try to fix things without having to have node.js and shifter enabled. They coudl then provide a patch, which the assignee on the ticket would be required to run through shifter.

Andrew
 
Average of ratings: -
Picture of Andrew Nicols
Re: YUI + shifter
Group DevelopersGroup Moodle HQGroup Particularly helpful MoodlersGroup Testers

> Then, if someone submits a patch that just updates the /src JavaScript, the integrators would be able to generate the necessary change to the /build JavaScript files.

I've updated the repository to add a new commit which allows exactly this:

  • in normal operation the /build directory is used
  • when $CFG->jsrev === -1 and $CFG->jsuseshifter === true then the /build directory is used
  • when $CFG->jsrev === -1 then the /src directory is used

I felt that jsrev was more appropriate than themedesigner mode for javascript development. I also felt that if you have knowledge of shifter's existence, then you should be the one to modify your config.php to avoid confusion for other users.

 
Average of ratings: -
Tim at Lone Pine Koala Sanctuary
Re: YUI + shifter
Group DevelopersGroup Documentation writersGroup Particularly helpful Moodlers

Yes, but the thing that sells it to me is the --watch option, so while you are doing development, you just edit the file and save, and it is automatically rebuilt.

 
Average of ratings: -
Dan at desk in Moodle HQ, Perth
Re: YUI + shifter
Group DevelopersGroup Moodle Course Creator Certificate holdersGroup Moodle HQGroup Particularly helpful MoodlersGroup Testers

but we're missing a few really great things: minification of our JS;

That's not true, when you have $CFG->cachejs turned on, we run it through minification.

 
Average of ratings: -
Picture of Amy Groshek
Re: YUI + shifter
Group DevelopersGroup Testers
You're right, Dan. But minification without lint is a double-edged sword. Currently minification fails silently on JS errors, and then whatever you had in your theme or mod after the error is not included in the file. This most commonly comes to light in theme development: theme designer builds theme with theme designer mode on, turns theme designer mode off (thus—and in many cases unknowingly—enabling caching), and theme promptly starts throwing obscure JS errors.

It's hard to take advantage of any of the new front-end development tools out there for CSS, JS, frameworks, etc without node.js. And it's going to get harder. IMHO Andrew is on the right track.
 
Average of ratings: -
Picture of Andrew Nicols
Re: YUI + shifter
Group DevelopersGroup Moodle HQGroup Particularly helpful MoodlersGroup Testers
That's not true actually Dan,

We currently do not minify Moodle YUI modules at all - see https://modules.lancs.ac.uk/theme/yui_combo.php?moodle/1354721791/calendar/eventmanager/eventmanager.js for an example.

The idea with a YUI module is that you use shifter (or it's predecessor) to create three versions:
* NAME.js
* NAME-debug.js
* NAME-min.js

The YUI loader configuration then tries to use the appropriate version depending on your settings. By default it would be -min.js and hence YUI minification is achieved.

Andrew
 
Average of ratings: -
Dan at desk in Moodle HQ, Perth
Re: YUI + shifter
Group DevelopersGroup Moodle Course Creator Certificate holdersGroup Moodle HQGroup Particularly helpful MoodlersGroup Testers

We currently do not minify Moodle YUI modules at all - see https://modules.lancs.ac.uk/theme/yui_combo.php?moodle/1354721791/calendar/eventmanager/eventmanager.js for an example.

Wow, I don't know why that is, but I think its a bug and should be fixed irrespective of this issue. smile

 
Average of ratings: -
Tim at Lone Pine Koala Sanctuary
Re: YUI + shifter
Group DevelopersGroup Documentation writersGroup Particularly helpful Moodlers

Well, Shifter seems to be offering us a fix for that, and other good things, using tools that are used by others in the YUI community, so you seem to be arguing for Shifter, providing we can provide a simple fallback for people who just want to tinker a bit. smile

 
Average of ratings: -
Picture of Andrew Nicols
Re: YUI + shifter
Group DevelopersGroup Moodle HQGroup Particularly helpful MoodlersGroup Testers

Wow, I don't know why that is, but I think its a bug and should be fixed irrespective of this issue. smile

Indeed, and the correct way to fix this is to use shifter. IMO it would be unwise to use another minifier; especially given the caveats of minification in general that Amy mentioned.

Such a change would not be back-portable (I don't think) so we should do it properly.

 
Average of ratings: -
Tim at Lone Pine Koala Sanctuary
Re: YUI + shifter
Group DevelopersGroup Documentation writersGroup Particularly helpful Moodlers

If the change to shifter is not back-portable, what does that do to back-porting JS bug fixes between master and stable branches? I guess there is a chance the cherry-pick will still work.

 
Average of ratings: -
Picture of Andrew Nicols
Re: YUI + shifter
Group DevelopersGroup Moodle HQGroup Particularly helpful MoodlersGroup Testers
I don't think that the shifter change is realistically back-portable - others may disagree but changing the way in which we structure code, and serve it seems like a fairly major change which should only be applied to a major version change, even if the content of that code shouldn't change.

Because of the file renames, I don't know if commits would be cherry-pickable - it's not something I've tried to be fair. I think we should definitely backport as many of the lint fixes as humanly possible - perhaps even fix them before we adjust the directory structure.
 
Average of ratings: -
Sam@moodle
Re: YUI + shifter
Group DevelopersGroup Moodle HQGroup Particularly helpful MoodlersGroup Testers
First up this is a very worthwhile discussion.

Really after reading through this thread I don't think the implementation is in question here yet. I've looked over it and I think there are a couple of things we still need to address there but all in all Andrew you've done an excellent job there.

I think what really needs to be decided is whether or not we want to dive down this rabbit hole and commit our selves to the first build process required by Moodle for development.
There are several advantages that having a build process brings that would not be otherwise achievable.
Really the highlights are:
# Native minification (remember we have three sources of JavaScipt: yui core modules, yui gallery modules, and moodle yui modules).
# Lint
# Automated tests

We'd benefit from the minification immediately, having lint easily runnable means we could, should we choose to, start to improve our JS code standard, and really tests would require much more work. Of course with tests it is much easier to write tests for new code than existing code so perhaps we could benefit early on from 2.5 development.

If we choose to adopt this build process there are other advantages. We'd be keeping up with YUI who is in the process of adopting this for their gallery I believe, and we'd be putting ourselves in a position to realise the benefits of having better code through use of tools that support better code development.

Its worth mentioning that we could probably integrate each of these separately and that while we may be able to avoid the need for a build process we would still require external tools, the implementation would be more complex and it would not be nearly so well self contained/organised. The build process ties all of there worthwhile features together.

The negatives... well lets face it the big one is you would have to build JS before using it.
Against that argument is that node.js is easy to install and well supported plus we have --watch.

Regarding integration I don't see any issues there, we'd perhaps have to decide whether we build everything as part of the release process, probably a good idea and is what YUI does themselves.

Adoption by developers should be fun, but nothing good documentation and support won't ease.

Certainly it would be master only, it would be a huge change and wouldn't be backportable.
I don't imagine changes will cherry-pick across nicely for those backporting bug fixes, unless git has some switch to allow fuzzy logic or user specificed path matching.. I don't know about any such thing but I havn't delved into that area of git either.

Personally, I would like to see a JS build process adopted and Shifter seems like the best choice given its YUI's own method of building.
The implementation is a whole other story, presently I'm happy just throw my vote into the "Yes I want it" box.

Of course this change would affect a lot of people, perhaps it is worth having a tracker issue for this so that we can go and vote?

Many thanks
Sam
 
Average of ratings: -
Picture of Petr Skoda
Re: YUI + shifter
Group DevelopersGroup Documentation writersGroup Moodle HQGroup Particularly helpful Moodlers
My +1 for shifter too. I would also vote to deprecate our old JS api such as module.js files.

I would personally prefer if every extra library or tool necessary for Moodle development was installed via the composer dependency manager (we could create our own packages if necessary).
 
Average of ratings: -
Picture of Andrew Nicols
Re: YUI + shifter
Group DevelopersGroup Moodle HQGroup Particularly helpful MoodlersGroup Testers

My +1 for shifter too. I would also vote to deprecate our old JS api such as module.js files.

I would love to deprecate the older JS APIs. We are still actively using many of the APIs which were deprecated some time ago too and it would be good if, once some of the above is decided and implemented, HQ could spare some time to work on dropping some of these uses.

 
Average of ratings: -
Picture of Andrew Nicols
Re: YUI + shifter
Group DevelopersGroup Moodle HQGroup Particularly helpful MoodlersGroup Testers
An excellent suggestion Sam, I've opened a new issue MDL-37127.
 
Average of ratings: -
Picture of Stuart Lamour
Re: YUI + shifter
Group Particularly helpful Moodlers

on projects that are not moodle i use http://gruntjs.com/ and recently http://yeoman.io/.

anything that can help me get a similar workflow in moodle gets a +1 from me.

 
Average of ratings: -
Picture of Colin Chambers
Re: YUI + shifter
 

Hi Guys, a +1 from me for better tools for JS development. It's a big challenge for us at the OU and it's set to grow. Either that or we're unable to implement major improvements that the rest of the web are implementing. 

Just installed and used jshint for MDL-32750 on andrews suggestion. Was really simple to install and use. Thought I'd record the steps. Tim suggested here. 

These instructions are for Windows 7 64 bit.

install node js  

go to nodejs.org click install. When installed windows -> start -> programs -> node.js to start command line

install jshint
with an open node.js command line from above type npm install jshint -g

Check code
navigate in command line to the root directory of your moodle install.

Call jshint with the config options and path to the file to check.

Andrew has a jshint config file that sets the desired moodle code standards for jshint to use

I first used the line below and got the error "unexpected token". 
Wrapped configs in double quotes to fix it e.g. "evil":false (Yep, jshint can be configured to be evil
Now you're ready. Check your code by typing...
jshint --config pathtoandrewsconfigfile pathtofiletocheck e.g.git\lib\editor\tinymce\yui\collapse\collapse.js

hth

 
Average of ratings: -
Tim at Lone Pine Koala Sanctuary
Re: YUI + shifter
Group DevelopersGroup Documentation writersGroup Particularly helpful Moodlers

I just did this (thanks Colin for sharing) and it just worked! Really quick and easy. You can get the configuration file from https://tracker.moodle.org/browse/MDL-37626, or more specifically https://raw.github.com/andrewnicols/moodle/12dec2cf3db8179f9af42079cb759a40424aa4b4/.jshintrc.

 
Average of ratings: -