Warning! proposed change to require_js

Warning! proposed change to require_js

by Tim Hunt -
Number of replies: 8
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
This has been planned for some time, if you read the Navigation 2.0 implementation plan carefully. But I thought I should give another warning before commiting this to HEAD (probably tomorrow morning).


Since Moodle 1.7, we have had a function called require_js, which made it very easy to make sure that you JavaScript files you wanted got linked to somewhere, and that each file was linked to no more than once.

That was very nice, but only went so far. For example, there was no equivalent require_css, and some of the YUI libraries need their own CSS files to work properly. Also, you could not control when the JS was linked to, and as discussed here, it is best to link to most of the JavaScript from the end of the HTML, not from <head> because that gives better page-load performance. Except, of course, some things cannot wait that long, so you need the option to do it sooner. And then, if your .js files are only linked to at the end of the document, but you need to call some function to initialise things, then that function call had better be at the end too.

So the requirements get a bit more complicated, and so to handle them, I have made a new class called page_requirements_manager to replace require_js. You use it like this:
$PAGE->requires->css('mod/mymod/styles.css');
$PAGE->requires->yui_lib('autocomplete'); // (1)
$PAGE->requires->js('mod/mymod/script.js');
$PAGE->requires->js('mod/mymod/small_but_urgent.js')->in_head();
$PAGE->requires->js_function_call('init_mymod', array($data))->on_dom_ready();
(1) Note that yui_lib knows that the YUI autocomplete library requires the other libraries 'yahoo-dom-event', 'datasource' and also 'autocomplete/assets/skins/sam/autocomplete.css'


Anyway, the code for page_requirements_manager is done, and currently attached to MDL-16695. If anyone would like to review it before I commit it tomorrow, that would be great. (I already had a lot of excellent feedback from Penny.) There are full PHPdoc comments and unit tests, which I hope make it easier to understand.

Note, I have not yet made a new, deprecated version of require_js for backwards compatibility; nor have I hooked this new class into print_header; nor changed all the places that currently call require_js. I will, but I was waiting in case people didn't like what I had done in page_requirements_manager.


And, if you are doing JavaScript-y thing in HEAD for Moodle 2.0, please be aware that this new API is coming along. I hope it makes you life easier.
Average of ratings: -
In reply to Tim Hunt

Re: Warning! proposed change to require_js

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
OK, this is now done, and all in CVS.

I though I had done a good job with the PHPdocs comments. However, it turns out that I did not know PHPdoc syntax as well as I thought I did, so you need to wait until tomorrow for http://phpdocs.moodle.org/HEAD/moodlecore/page_requirements_manager.html to be regenerated, before you look at it.

I have also updated Development:JavaScript_guidelines, which I think should now be considered part of the coding guidelines. However, as they are new comments are welcome. It is not too late to change them.


Average of ratings: -
In reply to Tim Hunt

Re: Warning! proposed change to require_js

by Dan Poltawski -
Great stuff, this looks really really nice.

I've got a few comments based on the javascript gudielines docs:

1) I really don't like two different behaviours of in_head() in this example changing depending on whether its being echoed..

// Load the js file from head.
// (This will throw an exception if head has already been output.)
$PAGE->requires->js('mod/mymod/scripts.js')->in_head();

// Loads the js file as soon as possible (from head, if that has
// not been output yet, otherwise output the script tag right here.)
echo $PAGE->requires->js('mod/mymod/scripts.js')->in_head();
However, having just looked at the code - I think thats a typo in the docs and should actually be ->asap() in the second example?

2) In the docs it says special syntax for language strings is the same as loading data in js, which seems easily confusing:

$PAGE->requires->data_for_js('tooltip', 'mymod');
However, once again looking at the code I think its another typo and meant to be ->string_for_js()?

(Didn't mean to post this to point out the mistakes in the docs, I didnt read the code until part way through post! The code seems to agree with what I think is best martin )
Average of ratings: -
In reply to Dan Poltawski

Re: Warning! proposed change to require_js

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
Yes, congratulations, you successfully found two serious typos in the docs. Fixed now.
Average of ratings: -
In reply to Tim Hunt

Re: Warning! proposed change to require_js

by Gordon Bateson -
Picture of Core developers Picture of Peer reviewers Picture of Plugin developers

Hi Tim,
well done with all this work on the page_requirements_manager class.

There is an intsy-wintsy problem that has crept in with the close_window() in lib/weblib.php (around line 718), as a result of using the new $PAGE->requires functionality.

The close_window() function uses the following code to try and print a javascript snippet and footer:

$PAGE->requires->js_function_call($function)->after_delay($delay);
print_footer('empty');

However, the 'empty' setting means that the print_footer() function aborts before printing the javascript, so the page doesn't close automatically as it is meant to sad

For a quick fix, you can call print_footer() without the 'empty' string, but in the long term it might be worth modifying print_footer() so that it only has a single exit point.

Probably you are already aware of this, but in case not I hope this message serves a useful purpose. I could add a tracker issue if that would be preferable smile

best regards
Gordon

Average of ratings: -
In reply to Gordon Bateson

Re: Warning! proposed change to require_js

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
No, the bug is that print_footer('empty'); does not call $PAGE->requires->get_end_code();

Thank you very much for catching this.


And the correct long-term fix is MDL-19077, which I am working on now - that is, print_footer is going to be completely rewritten.
Average of ratings: -
In reply to Tim Hunt

Re: Warning! proposed change to require_js

by Gordon Bateson -
Picture of Core developers Picture of Peer reviewers Picture of Plugin developers

Wonderful! Thanks for the info and keep up the great work!!
Gordon

Average of ratings: -
In reply to Tim Hunt

Re: Warning! proposed change to require_js

by Myles Carrick -
Tim,
This is awesome. It's so great to see how you worked with so much consultation on this over such a long period to get it right - nice work!
Myles C.

Average of ratings: -