Split javascript files in media into modules

[ericlewis]

​Yesterday in dev chat, ideas for splitting up distinct javascript modules in were discussed. As we move towards more and more javascript-heavy administrative interfaces, this will prove quite helpful in readability, and by extension maintainability. A 6000 line file made up of 50-some views is not easy to step into. A folder of well organized modules for one feature would be.

This will only affect the src/ directory in the develop repository, which will use our build process to concatenate into the build/ directory, so nothing will change for developers using the build/ directory or the core repo.

We'll start this effort with media, and end up with a standardized way of doing this going forward.

[wonderboymusic]

browserify.diff​ breaks up the Backbone classes in media-models.js, media-views.js, media-audiovideo.js, and media-grid.js into CommonJS modules and injects them via Browserify on build/watch into a built file. Let's start at the beginning.

Brain overload

Files that are 1000s of lines long are hard to consume. We try to alleviate this by adding copious amounts of docs. Still, it's a lot to look at. Ideally, we would break our files into smaller modules and then somehow join them together in a build process.

Require vs Browserify

Require is a great tool for converting AMD modules into built files. Require leans on Dependency Injection in its syntax:

define([
    'model/taco',
    'model/burrito', 
    'controller/meal'
], function (Taco, Burrito, Meal) {
    var Dinner =  Backbone.View.extend({
        // taco-related code
    });
    return Dinner;
});

This syntax works great, unless you have way more dependencies. Refactoring code could unwind a module that has a lot of dependencies, but if you are just trying to convert legacy classes into a module, Require starts to get a little weird.

Require becomes a Grunt task to makes one big file by recursing the dependency tree in an initial manifest. Require, by default, loads JS asynchronously, which can cause race conditions with plugins or themes that expect code to be registered on $(document).ready() or window.onload.

Require works even if you don't build via Grunt.

Browserify is a tool that allows you to use Node-style CommonJS modules and run them in a browser without changing from the Node syntax. Browserify requires a build for this to work.

Using our example from above, this is the syntax for Browserify:

var Taco = require( './models/taco.js' ),
    Burrito = require( './models/burrito.js' ),
    Meal  = require( './controller/meal.js' ),
    Dinner;

Dinner = Backbone.View.extend({
    // taco-related code
});

module.exports = Dinner;

Browserify leans more towards the Service Locator pattern.

Browserify scans the abstract syntax tree (AST) of your JS code to compile dependencies. Your modules themselves get wrapped in their own scope like so: (function(require,module,exports){ .....YOUR_MODULE..... }).

After spending a lot of time messing around with both: I think we should use Browserify.

Converting "Legacy" Code

The media JS code is some of the most "modern" code in WordPress, but it still clunkily lives in huge files. To convert the code into Common JS modules, we need to make a lot of individual files (one for each Backbone class). We also need to make sure we maintain the existing wp.media namespaces for backwards compatibility. We don't want any existing functionality to change, we just want to build the files differently.

Since we are in experimentation phase, I have added a folder to wp-includes/js, media, that contains the modules and the built manifests. My patch adjusts script-loader.php to use these new paths.

media contains the following files/folders:

controllers/
models/
router/
utils/
views/ (with another round of subfolders)
audio-video.manifest.js
grid.manifest.js
models.manifest.js
views.manifest.js

The build pipeline

First things first, run npm install

*.manifest.js files get built into *.js files when you change a file in media/*, provided you are running the grunt watch task. The watcher will automatically call browserify:media and uglify:media when those files change. This allows you to run with your site from src or build , and you will still get Browserify'd files. SCRIPT_DEBUG will either run *.js or *.min.js, just like any other minified JS in core.

This is an experiment

I would like us to do something like this. I got the idea yesterday to do this proof of concept and just ran with it. Consider this a request for comments.

[iseulde]

options: {
	debug: true,
	keepAlive: true,
	watch: true
}

for the browserify task seems to be a lot faster than adding it to the watch task.

[ericlewis]

Creating /controllers/states/ and /controllers/frames/ folders might be helpful, since we have a bunch of both.

[wonderboymusic]

In 31373:

Split the media JS files into modules:

• Add a new folder in wp-includes/js, media
• Create manifest files for views, models, grid, and audio-video
• Make browserify an npm dependency
• Add Grunt tasks for browserify and uglify:media on build and watch
• Update the paths loaded for media files in script-loader
• All new files were created using svn cp from their original location

Please run npm install. While developing media JS, you must run grunt watch.

See #28510.

[wonderboymusic]

In 31379:

Make sure that media/views.js doesn't load Models that already exist in media/models.js.

See #28510.

[wonderboymusic]

In 31380:

Bind this at calltime instead of letting self spill down into closures.

See #28510.

[iseulde]

Above patch adds newlines at the end of all the files, and removes some trailing whitespace.

[iseulde]

Added JSHint for the media files. Fixed some errors. Includes previous patch.

[iseulde]

Same, but with updated bundle. A con of browserify is that is bloats patches... :( But necessary if we want trunk to be runnable...

[iseulde]

I removed the "debug" option for now, because that doesn't do anything. If we want that, we need to add it under browserifyOptions. But patches should never be uploaded, or committed, with that option. It adds the absolute path to all the files, which is different for everyone of course. Not sure how we should solve this. Normally you don't version control the built files. When "watching", we could add that option, but before uploading the patch the contributor should run browserify without it.

[iseulde]

About the watch task. Imo we should really use Browserify's build in "Watchify". It's a lot faster because it caches previous work. Just running grunt watch:browserify on my computer takes about 80s each time a file changes. This would need to be a separate watch task though. You can still add the grunt watch task after it and do more stuff, and leave out the keepAlive option for Browserify.

I'll wait to make a patch, don't want to make a huge patch combining the previous one.

[iseulde]

Cleaned up the manifest files a bit. It's already scoped by Browserify.

[wonderboymusic]

In 31385:

Media JS files:

• In media manifests, ditch IIFEs and global injection, these get dynamically scoped via Browserify
• Remove the debug option from browserify:media
• Add jshint:media to jshint:corejs
• Add a trailing newline to all new module files

Props iseulde.
See #28510.

[iseulde]

Here's what the Gruntfile could look like with Watchify. You can run grunt watchify to watch all browserify bundles (I had to split them, I couldn't get it to work otherwise). The last task is kept alive, but you can just as well use grunt watch for that. You can watch the Browserify bundles (instead of the modules themselves) to run any tasks after Browserify did its thing (e.g. uglify).

I also added browserify to the precommit task so you can use it to remove debug information.

[iseulde]

Just removed :media form the build task.

[wonderboymusic]

In 31393:

Export the proper class in media/views/image-details.js

See #28510.

[SergeyBiryukov]

In 31399:

Set svn:eol-style for [31373].

see #28510.

[wonderboymusic]

In 31405:

Remove unused media JS files.

See #28510.

[wonderboymusic]

In 31487:

Restore the changes I made in [31016] for chromeless Vimeo that were missed in [31373].

Make sure that the build file for grid doesn't load views/media-details.js, it already exists in the audio-video build.

See #29267, #28510.

[wonderboymusic]

In 31489:

The grid build should not load views/edit-image, it already exists in the views build.

views/edit-image.js doesn't need to set a render method if it doesn't override its parent.

See #28510.

[wonderboymusic]

In 31490:

After [30120], in media JS files, move from @constructor to @class annotations as per JSDoc preferred nomenclature: ​http://usejsdoc.org/tags-class.html

See #28510, #30193.

[wonderboymusic]

In 31491:

Specify globals in media JS files - it is important to denote where we are diverging from dependency injection.

See #28510.

[wonderboymusic]

In 31492:

Media JS files:

• specify globals in more files
• add missing wp.media.* namespace docs
• add doc blocks to files that had none

See #28510.

[wonderboymusic]

In 31493:

Make sure the audio-video build does not load files from the views build.

See #28510.

[wonderboymusic]

In 31494:

Make sure the grid build does not load files from the views build.

Fix the errant back-compat assignment for wp.media.view.Frame.

See #28510.

[wonderboymusic]

In 31618:

Media modules: set $ to Backbone.$, instead of jQuery, so fewer globals are imported.

See #28510.

[helen]

@iseulde Does your patch for watchify still need to be examined?

[iseulde]

Yes, I'd like us to consider that.

[wonderboymusic]

In 31629:

Use browserify's watch mechanism in tandem with grunt watch. Makes building media bundles about 10 (million) times faster.

Make the browserify config more dynamic to make future additions easier and to ensure proper use of SOURCE_DIR.

Props iseulde, wonderboymusic.
See #28510.

[wonderboymusic]

I'm going to mark this as fixed. New tix for no problems.

[dglingren]

I am the author of ​Media Library Assistant, a plugin that adds some features to the Media Manager Modal Window. I have struggled with earlier versions of the media JS code and I am happy to see these improvements. However, the 4.2 beta completely breaks my plugin and I am at a loss on how to modify my code to support the new version. Where can I go to find the "right" way to, for example, add a control to the toolbar or to replace the existing search box with an enhanced version? If there is a better place to ask this question, let me know. Thanks for any help you can provide.

[wonderboymusic]

Oh man - I will take a hard look and see what is necessary here

[dglingren]

Thank you for your instant response! Let me be clear - I am all in favor of the improvements you've made and I know it's my responsibility to update my code to conform. I admit my JavaScript, etc. skills lag my PHP skills and my current Media Manager support is a hack; I just want to know how to do the right thing.

By the way, my code still runs fine (mostly) with the Media/Library grid view, but it fails completely in the popup "Add Media" modal window.

Thanks again for your consideration.

[kraftbj]

Testing out Jetpack's Tiled Galleries (which adds a menu to the Create Gallery modal), that is also no longer displaying.

[wonderboymusic]

In 31935:

Let us pray to the gods of backwards compatibility:

• The way that the JS modules for media are currently set up turns the existing global wp.media namespace into a read-only API, this is bad.
• For the existing module implementation to work with plugins, those looking to override or extend a class would have to modify their own plugin to use browserify - we can't expect this to happen
• Because the general way that plugins override media classes is via machete (resetting them to something else), we cannot use require( 'module' ) in the internal code for media modules

We CAN continue to use require( 'fun/js' ) in the manifests for media.

Future code/projects should carefully consider what is made to be public API. In 3.5, EVERYTHING was made public, so everything shall remain public.

See #31684, #28510.

[DrewAPicture]

@wonderboymusic: What's left here after [31935]? Can we call this fixed?

[DrewAPicture]

Per @wonderboymusic in Slack, let's call this fixed, with thanks for the back-compat excellence in [31935].

[dglingren]

I am relieved to report that the latest changes have restored my plugin (and probably others) to working order. I look forward to the day when I can lay down my machete and go back to the scalpel. Thank you for your great work!