Posted by mfer on December 31, 2008 at 4:45pm
63 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | javascript |
| Category: | task |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | Assetic, frontend performance, JavaScript, Patch Spotlight, Plugin system |
Issue Summary
Problem/Motivation
Currently both the JS and CSS preprocessors do not work for all use cases. Since drupal_build_js_cache() preforms the JavaScript preprocessing to roll all of the js files into one file, developers try to alleviate this with other methods such as the sf_cache module.
It has been proposed to make the JS and CSS preprocessors pluggable, possibly with the help of assetic. To be more specific Owen Barton suggested 2 interfaces, a bundler and a packager.
Proposed resolution
- We make the JS and CSS preprocessing pluggable with two separate interfaces, the bundler and the packager.
- We also integrate assetic into core as the packager.
Remaining tasks
- We need to reach a consensus on if we want to actually integrate assetic into core
- We also need to create a patch for making the JS and CSS preprocessors pluggable
- If assetic was voted to be integrated with core we need to either
- Create a patch here or
- Create a separate issue for assetic's integration into core
Comments
#1
Here is my first pass at a pluggable JavaScript preprocessor.
#2
Updated the interface name to JSPreprocessingInterface for consistency and fixed some comments.
#3
Looks pretty good to me. Some formatting issues:
+ *+ * @param $files
+ * An array of JavaScript files.
+ *
+ * @return
+ * html for preprocessed JavaScript files.
need to indent two spaces under the @s. That goes for just about all of the PHPDocs.
I know these bits are just getting moved but that's no reason not to improve them:
+ // Create the js/ within the files folder.I'd say "Ensure there's a js directory with in the files directory that it is writable."
+ file_check_directory($jspath, FILE_CREATE_DIRECTORY);I think you should also make it writable:
+ file_check_directory($jspath, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS);#4
Thanks for the feedback, drewish. Updated.
#5
The last submitted patch failed testing.
#6
Previous patch still applies with no issues for me.
#7
Any chance you can name preprocessing to something else? The theme system has kinda taken over that name.
#8
@moshe weitzman - I'm open to a name change. Drupal 6 already uses preprocess when refering to JavaScript so it would be a name change from an existing convention. Though, this topic is out of the scope of this patch which is just to take an existing system (with an existing name) and make it pluggable. It's primarily a re-factoring of code.
#9
Stuck up a note about this issue in the JavaScript Aggregator module: #356596: Javascript Aggregator » D7 port.
Would be nice to have a test case. I'll see what I can do....
#10
Had to make the preprocessor able to lazy load classes from different files. This meant moving JSPreprocessingInterface to common.inc, and changing "preprocess_js_system" to:
<?phparray(
'name' => 'DrupalPreprocessJS',
'file' => 'includes/preprocess.inc'
)
?>
....When the preprocessor is loaded, it will check "file" and include that file before creating the interface.
As for the tests, I got pretty far, but couldn't get it to work all the time. Help appreciated.
#11
....... Forgot the patch.
#12
@rob - thats D6 style code. We no longer have to specify files, with the code registry.
#13
Why are we telling it what file to load? The registry takes care of loading files for classes and interfaces. If it's in the includes folder it just finds it. If it's a module you define what module files to scan. See http://drupal.org/node/224333#registry and http://api.drupal.org/api/group/registry/7.
#14
Fixed the tests, but am having troubles with the registry. Assistance would be nice.
#15
Ahhh, had to add common.test to the registry.
#16
I updated the test to remember the configured class for the JS Preprocessing and restore it after the test.
2 Questions:
#17
I'd love for it to be referenced somehow else, but can't think of anything since it's running on a different process and the class isn't loaded. Any other ideas would be awesome.
I don't think it really matters, but we could move it to a different class if that's prefered.
#18
The attached patch moves including common.test to a javascript_test module in the tests folder. The javascript_test.info file includes the module. The javascript_test.module file is a dummy file. If we change core to not require module files this can go away. The setup function for the javascript tests enables this module.
#19
This is looking pretty much RTBC, but Crell just posted #363787: Plugins: Swappable subsystems for core, so I think we should see how it grows...
#20
Great work! The same should be applied to CSS preprocessing.
Subscribing.
#21
#22
nice, subscribing
#23
I don't think the plugins/handlers system will make it into D7. So, here is an update to the previous series of patches that store the class in a variable.
This is the overall layout of the functionality but the tests could use some work.
#24
The last submitted patch failed testing.
#25
Rerolled the patch. Didn't test it, just a straight re-roll.
#26
The last submitted patch failed testing.
#27
Sub
#28
#29
subscribing.
#30
Subscribing
#31
Agree with #20 that CSS should be included, so updating title
#32
subscribing
#33
Subscribing.
#34
Subscribing.
#35
+++ includes/common.inc 31 Aug 2009 17:13:02 -0000@@ -3401,44 +3401,68 @@ function drupal_add_tabledrag($table_id,
- * @param $filename
- * The name of the aggregate JS file.
- * @return
- * The name of the JS file.
+ * Returns a JavaScript preprocessing object that implements
+ * JSPreprocessingInterface.
Should be one line of comments rather than two.
+++ includes/common.inc 31 Aug 2009 17:13:02 -0000@@ -3401,44 +3401,68 @@ function drupal_add_tabledrag($table_id,
- $contents .= file_get_contents($path) . ';';
+function drupal_js_preprocessor() {
+ static $instance;
+ if (empty($instance)) {
+ // Retrieve the preprocess system.
So excited to see what this spawns in contrib :-) .
+++ includes/common.inc 31 Aug 2009 17:13:02 -0000@@ -3401,44 +3401,68 @@ function drupal_add_tabledrag($table_id,
+ $class = variable_get('preprocess_js_system', 'DrupalPreprocessJS');
+
+ // Allow for lazy loading of the class.
+ if (drupal_autoload_class($class)) {
+ $interfaces = class_implements($class);
+ if (isset($interfaces['JSPreprocessingInterface'])) {
+ $instance = new $class;
+ }
+ else {
+ throw new Exception(t('Class %class does not implement interface %interface', array('%class' => $class, '%interface' => 'JSPreprocessingInterface')));
Is it just me, or does DrupalPreprocessJS not exist in this patch?
+++ modules/simpletest/tests/common.test 31 Aug 2009 17:13:03 -0000@@ -871,6 +874,23 @@
+ function testPluggablePreprocessor() {
+ // Switch the preprocessor.
+ variable_set('preprocess_js', TRUE);
+ variable_set('preprocess_js_system', 'testJSPreprocessing');
+
+ // Add some JavaScript to test the preprocessor system.
Some tab characters.
5 days to next Drupal core point release.
#36
Subscribe
#37
sub.
#38
Not sure where I found this, but it definitely looks awesome: https://github.com/kriswallsmith/assetic
#39
#1332580: Clean up API docs for color module is adding docblock headers to JavaScript. That is good for docs, but without progress on this issue we are looking at adding 2-3kb to every request to Drupal if contrib modules follow suit since we don't touch comments at all with js aggregation at the moment.
Also issues like #865536: drupal_add_js() is missing the 'browsers' option and the very different alters I had to do in http://drupal.org/project/agrcache show that our css and js handling are woefully out of sync. So bumping priority of this. Adding comments to JavaScript files should not be an (admittedly small) performance regression.
#40
After all the work on #865536: drupal_add_js() is missing the 'browsers' option I created a new ticket #1332626: Remove duplicate code between all the css/js function such as the aggregation, grouping, etc about combining the all the JS/CSS grouping, aggreation etc.
Would it makes more sense for that to be done here? I figured we can combined them 1st, and then this would be a lot less work.
#41
@Rob Loach it doesn't look like Assetic does aggregation of files. Is this the case?
#42
This can finally go somewhere: #1497366: Introduce Plugin System to Core go nuts people! :)
#43
It aggregates the files by default.
<?php
use Assetic\Asset\AssetCollection;
use Assetic\Asset\FileAsset;
use Assetic\Asset\GlobAsset;
$js = new AssetCollection(array(
new FileAsset('/path/to/another.js'),
new FileAsset('/path/to/different/file.js'),
));
// the code is aggregated when the asset is dumped:
echo $js->dump();
?>
For our use, we would probably need to $js->dump() into the aggregated file.
#44
@Rob Loach I see that. What do you think of this for a path forward.
If this sounds good we can start coding.
#45
I would really really like to see this moving forward, ping me when reviews are needed, don't have much time to work on a patch for now.
The plan looks great to me. I'd just like to see/confirm where we're going. Currently we have the following functions to handle JS (same for CSS) in common.inc:
drupal_add_js
drupal_get_js
drupal_sort_css_js
drupal_js_defaults
drupal_group_js
drupal_aggregate_js
drupal_build_js_cache
drupal_clear_js_cache
_drupal_flush_css_js
So I guess what i'm really asking is what could be the API that is going to be implemented and what will it cover?
#46
I actually think we need (at least) 2 main interfaces of pluggability:
- A "grouper" (or "bundler" perhaps?) defines the groups and ordering, given input/hints about what has been requested on the current page and potentially other things it knows.
- A "packager" which actually does the filtering and concatenation work.
The main reason for this is that the "grouper" really needs to operate in the HTML page request, since it needs to know all the assets the page needs fairly holistically and needs to be able to affect the page HTML to put in tags in the right order and with the right attributes - the "packager" however, needs to be (perhaps optionally) able to live in a separate page request for lazy generation of assets (imagecache/advagg style) - passed in a list of assets, or a reference to a list of assets, it caches the file and returns it to the browser.
Assetic is really a "packager" plugin and I think keeping that interface separate is an ideal layer of abstraction - much of our own complexity is because we have mixed up parts of the grouping and packaging operations (of course Assetic is pluggable itself on a more fine grained level, which is great too). Also, there is lots of benefit from being able to switch out the grouping subsystem separately from the packaging subsystem - as we know it is hard to have a single grouping algorithm get optimal groupings for all sites.
#47
I'd love to see an expansion of comment #44 as an issue summary. It's not clear (to me) how the plan will be implemented and how people can help out.
I don't want this issue to slip until D9. :-(
#48
#46 is spot-on.
#49
Tagging for issue summary
#50
Yep, completely agreed with #46, they're two separate systems and should both be pluggable.
#51
ah forgot to remove the issue summary initiative... done!
#52
I don't speak code, but I think that the assetic part should be its own issue. Just to keep issues clean. After all, it's not a part of this issue (yet), just a separate think that would make it easier on us ;)
...I'd start a new issue for that myself, but we'd better leave this to somebody with more knowhow. If a new issue is created and we reach consensus that assetic should be in core, then we should perhaps postpone this while waiting on that.
#53
If we're voting +1 for assetic
#54
I'm wondering how to reconcile this statement with what's being proposed in #1542344: Use AMD for JS architecture
#55
Maybe that can help?
http://drupal.org/node/1667500
http://drupal.org/node/1667512
http://drupal.org/node/1667514
#56
Plugins are in, who is interested to work on that? This is borderline critical for us.
Ping me on IRC/email/smoke signals/whatever if you're interested and I'll get the stuff needed to get started.
#57
Hey, nod_, If I can learn enough of the plugin system I want to help with this.
#58
All right, so we have cosmicdreams, SebCorbin and possibly Rob Loach interested in getting this going. I'll try to see how we can get that going :)
#59
I am interested in working on this also - perhaps we should schedule an IRC chat sometime to hash what the interface looks like in a bit more detail?
#60
Had a great weekend at MDS and saw how a plugins are made. EclipseGC has converted a number of our standard blocks over to the plugin system. I think that should be either in his sandbox or in a patch somewhere. I try to track it down and post it here.
In short, its crazy awesome/simple.
#61
Since we're interested in using the plugin system, tagging for aggregation.
#62
Also, Figuring out the class interface is definitely the first step. Probably would make sense to have 2 interfaces and 2 plugin types, one for CSS and one for JS just for sanity. If that IRC meeting happens just ping me ahead of time so I know to be there :)
#63
Very interested in this. If I can help, I will.
#64
Trying out an implementation with Assetic #1751602: Use Assetic to package JS and CSS files help welcome :)
#65
Please note #46 though - Assetic is only half of what we want to make pluggable, I think (though I think it is a good basis for that half!).
#66
I've attached a diagram of how this might work. I've discussed it with @sebcorbin who thinks it should work with the diagram here: http://drupal.org/node/1667500
I've also attached a not-to-be-tested patch with some code I was was working on yesterday to move much of the CSS/JS preprocessing code to an overridable class, based on how the cache system works. It's pretty scrappy and I'm going to rewrite it, it's just here mainly to show that progress is being made.
#67
Awesome, keep up the good work!
#68
Another work-in-progress patch.
Same functionality as before but now using the core Dependency Injection Container (thanks for the help, @alexpott).
Changed the AssetManager to an abstract class rather than an interface and renamed things generally.
Setting to needs review just to let the test bot find issues. Lots of work still on-going.
#69
#70
The last submitted patch, pluggable-asset-management-352951-68.patch, failed testing.
#71
Registered the bundle in install.core.inc
Hopefully will work…
#72
The last submitted patch, pluggable-asset-management-352951-70.patch, failed testing.
#73
Looks like it's the call to
drupal_get_css()inajax_render()which is causing the fails.#74
Small amends to keep up-to-date with core and to (hopefully) fix the ajax_render() fails.
#75
The last submitted patch, pluggable-asset-management-352951-74.patch, failed testing.
#76
Re-roll.
update.php and authorize.php weren't aware they needed to know about the css_asset_manager class, so I told them.
Fingers crossed.
#77
After discussion with sebcorbin, going to move development of this and the Assetic work into a sandbox, which I hope to set up in the next day or two.
I'll post details here. Just ping me if you want access.
#78
Assetic was added to Drupal core here: #1784774: Add Assetic component to core
#79
#80
Sandbox at http://drupal.org/sandbox/mcjim/1813618
#81
Took a pass at adding a JS asset manager. It's basically untested and still a bit wonky (we need to find tune the methods on the abstract class, and I think the AJAX pre-render stuff just needs to go somewhere else), but I tried not to actually change anything functionality wise. WIP regular and branch patch attached.
#82
I don't think the JsAssetManager class is in the patch.
#83
This is my first look in a long time to this, I like the idea, some small comments:
+++ b/core/includes/common.incundefined@@ -3873,81 +3848,19 @@ function drupal_js_defaults($data = NULL) {
+ $javascript = drupal_container()->get('js_asset_manager');
+ return;
+ $javascript->set($js);
the return looks strange?
+++ b/core/lib/Drupal/Core/Asset/AssetManagerFactory.phpundefined@@ -0,0 +1,55 @@
+ // Need to return other config, too, bundler and packager variables for each type. ¶
comment too long and trailing whitespace
#84
The last submitted patch, 352951-js.patch, failed testing.
#85
Re-roll of patch in 76 to keep up with HEAD.
Now looking at a re-roll of 81.
#86
So what's left to do here? It looks like we still need a way to define pluggable transformations to apply to the assets that the asset manager knows about (minify, compress, aggregate, etc) and the Javascript asset manager is missing from #81. mcjim, do you have a todo list that you're working from here? I'd love to spend some time helping to move this forward.
#87
I had a look for the JsAssetManager and it appears to be kaput. Shouldn't be too hard to recreate it though.
In terms of what is remaining in the big picture, I think the answer is "a lot". At BADcamp I spoke with several people (and had a long call with sdboyer on Friday) and there seems to a lot of agreement around refactoring the processing so that the grouping and preprocessing happen at a "build" stage, where they can get a holistic view of what assets are being registered and what kind of context (e.g. paths, blocks, permissions) they may be needed in, rather than during a random page build which provides insufficient information to actually group sensibly and causes no end of issues with concurrent building on busy sites. This switch is pretty much dependent on getting AMD or equivalent in (so we can include JS without enabling it), and probably a few other things (mechanism for blocks to pass their asset needs from blocks in the response, and probably some kind of introspection mechanism for blocks) - I am working on a summary.
In the mean time I think this is still a beneficial change - getting our current grouping logic into a plugin and (as a second step) converting the existing preprocessing to use Assetic (which is a separate issue) is all stuff we need to do anyway.
#88
I'm not sure what this patches are all doing here, but could you guys make sure the packager is re-usable as stand alone function for modules, please. See #1825434: Remove whitespace characters in Code snippet (before) for the reasons.
#89
#85: pluggable-asset-management-352951-85.patch queued for re-testing.
#90
The last submitted patch, pluggable-asset-management-352951-85.patch, failed testing.
#91
is this related to or impacting #886488: Add stampede protection for css and js aggregation ?