A patch straight from #1542344: Use AMD for JS architecture that declare all JS files as library and add the relevant dependencies to all scripts.
SebCorbin should be credited, he's the one who wrote everything, I'm just putting it in a patch.
This will greatly simplify issues like #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page as well as prepare core for a module-oriented JS.
Basically, we shouldn't be using drupal_add_js
anymore, all scripts should be declared in library_info
since pretty much all of them have dependencies on drupal and jQuery. As D8 is focused on mobile, it's critical to be able to add JS that is not implicitly adding jQuery and drupal.js to the page.
The naming isn't totally consistent, feedback welcome. There is still one drupal_add_js left in the color module. not sure how best to fix it yet.
Comment | File | Size | Author |
---|---|---|---|
#113 | core-js-explicit-dependencies-1737148-113.patch | 74.52 KB | nod_ |
#111 | core-js-explicit-dependencies-1737148-111.patch | 74.25 KB | nod_ |
#107 | core-js-explicit-dependencies-1737148-107.patch | 74.26 KB | nod_ |
#107 | interdiff.txt | 3.55 KB | nod_ |
#93 | js-deps-1737148-86.patch | 74.54 KB | nod_ |
Comments
Comment #2
tim.plunkettPart of locale_library_info_alter() was off.
Comment #3
nod_All right, awesome new patch that actually fix #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page.
There are a few things related to ajaxPageState moved around, this should be using #1573020: Use data-* for CSS and JS files list to begin with so that we don't even need to do that in PHP.
Comment #4
nod_Thanks, rerolled with changes from #2
Comment #5
nod_Bumping since it takes care of a nasty bug.
Comment #6
Wim LeersI *think* my review over at #1542344-44: Use AMD for JS architecture has already been incorporated?
Comment #8
nod_Putting back to major and NR since testbot is going crazy.
Yes the review has been addressed by SebCorbin before I made the patch, check the AMD sandbox log :p
Comment #9
nod_Comment #11
tim.plunkettNot sure why its failing exactly, but a note for the next reroll:
This should come before locale_library_info_alter(). Also, adds an extra blank line by mistake.
Comment #12
nod_Added more dependencies.
On patch without the local changes to see if that's the only thing crashing the tests.
Comment #14
nod_All right, should just have the tests to fix.
SebCorbin found out that variable_get was called during install, before DB was set, Added a check for the MAINTENANCE_MODE . Apologies for blaming testbot :)
Comment #16
nod_Less test failures.
Comment #17
Wim LeersQuick comments review. Quite a lot of comments to explain tricky edge cases, great! :)
Remove "those", no?
Sort the JavaScript files so that they appear in the correct order.
s/Ajax/AJAX/
s/or/otherwise/
s/setting/settings/
s/TODO/@todo/
Comment #18
nick_schuch CreditAttribution: nick_schuch commentedUpdated comments as per #17.
Comment #20
pounardYou don't include settings if they are no other JS on the page, but what about arbitrary inline JS or potential external JS being included?
Comment #21
nod_well, it just works™
Everything is managed by libraries so to get settings, you need to
drupal_add_library
thedrupal
library, otherwise, nodrupal.js
and noDrupal.settings
. The rest works. But yeah we still have to keepdrupal_add_js
around for now. That said you can add this type of JS in a library hook too.Comment #22
pounardAll right, thanks.
Comment #23
tim.plunkettIt seems the last patch was made by editing the previous patch directly, and the grammar fixes were made to both the deletions and insertions. Which doesn't work.
Straight reroll with interdiff against #16.
Comment #25
nod_Green :D
All right changed a couple of things:
- drupal.settings needs to be added as a dependency explicitly as well, that's the same mess otherwise.
- there is a js_alter hook in system to add the default configuration for the JS setting data, it's messy to try to do that in drupal_add_js
This fixes #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page the right way.
Comment #27
nod_Here you go
Comment #28
nod_Should get rid of the warnings at least, added an isset() in locale_library_info()
I moved things around too much, the ajax thing is broken.
Comment #30
Wim Leerss/JS/JavaScript/
The comment says "if there is no other JavaScript". But here it's checking for other JavaScript *settings* specifically.
I think the comment is right and the code is wrong. The original code also just checked for $items not being empty, not $items['settings'].
However, if I look at drupal.ajax, which is where ajaxPageState would be used (primarily), then I see that it explicitly lists a dependency on drupal.settings. That means this check should also work.
I'd love a clarification here :)
"removed below"? Not within this function; so it's better to refer to the specific function in which this is happening.
Wrap at col 80.
Maybe we should assertFalse ()these?
So drupal.settings gets added because we're adding settings that would otherwise not appear.
Can't we automatically add drupal_add_library() whenever drupal_add_js(array(), 'setting') is called?
Or do we intentionally not want to do this to enforce "good behavior"?
s/tracker/language/? :)
Drupal-specific JavaScript.
I LOVE THIS!
I think you should extend this comment and state that this is ONLY necessary for JS that is added through drupal_add_js(), i.e. for JS that is not added as a lib, because JS libs should specify a dependency on drupal.settings, in which case this hook implementation is not necessary at all.
Unnecessary/unwanted leading spaces.
Comment #31
larowlantackling Wim's comments and last two failing tests.
Comment #32
larowlanAttached patch fixes three of the failing tests.
Adds new util function drupal_get_content_request_type which is wrapper to DIC to get the current request type.
Previous patch didn't load/update $settings['ajaxPageState'] because no other js settings existed for the ajax framework tests, during lazy load.
New patch adds a check to see if it's an ajax request, if so $settings['ajaxPageState'] is always added in drupal_get_js (i.e. of course we need to load javascript in an ajax response).
Interdiff attached too.
Comment #33
larowlanLetting the test-bot have a look.
Note there is still an issue with the lazy loading of scripts.
You can see response.data =
In the ajax insert command, however it is adding an empty <div> to the page instead of the script in it's own right.
The problem lines are in ajax.js around here:
After this runs, new_content_wrapped only has an empty text child element: i.e. new_content_wrapped.html() =
'<div></div>'
Comment #34
larowlanComment #36
nod_#33 is a known issue #736066: ajax.js insert command sometimes wraps content in a div, potentially producing invalid HTML and other bugs
Here's me hoping :)
Comment #38
nod_#36: core-js-explicit-dependencies-1737148-36.patch queued for re-testing.
Comment #40
larowlanre #33 is a known issue, true but without this patch the lazy loading of script tests work so I think we're missing something
Comment #43
nod_Yeah got to find out what it's about, it seems to work fine for me. The script is actually added to the page.
In the meantime, reroll.
Comment #45
Wim LeersEverything I said in my review #30 was addressed, except for the items below.
Maybe we should assertFalse() these?
So drupal.settings gets added because we're adding settings that would otherwise not appear.
Can't we automatically add drupal_add_library() whenever drupal_add_js(array(), 'setting') is called?
Or do we intentionally not want to do this to enforce "good behavior"?
Comment #46
nod_I expclicitly don't want that. It's was part of the reason why the drupal_add_js code was weird. If you want settings, declare it.
Comment #47
Wim Leers#46: Okay :) I personally agree, I'm just trying to approach it from all angles. But, in that case, a small explanatory comment would be nice, because you won't see this happening in many locations (since everything/as much as possible should move towards hook_library()).
Regarding the test failures:
Failed to find test tables to drop.
Hence it seems the problem lies in Locale module/SimpleTest module. The discrepancy between what I see on a fresh local install vs. what qa.drupal.org says is actually worrisome, I think. I've requeued the testing to hopefully get the same result.
Comment #48
Wim Leers#43: core-js-explicit-dependencies-1737148-41.patch queued for re-testing.
Comment #50
nod_#43: core-js-explicit-dependencies-1737148-41.patch queued for re-testing.
Comment #52
Wim Leersnod_: do you have any more ideas? Does my research from #47 give you any ideas what might be causing this?
Comment #53
larowlanI too grappled with the locale fails.
If you do exactly what the test is doing in the UI, it passes - you get the exported file.
But the test is getting the 'nothing to export' message from line 248 of locale.bulk.inc.
Perhaps it's a profile thing, if I get time today I'll try and install the testing profile and test in the UI.
Comment #54
nod_Tried with the testing profile installed, get "Nothing to export" doing the thing from the UI.
Comment #55
larowlanToo damn efficient - the earlier patches resulted in no calls to drupal_add_js so _locale_parse_js_file() never ran, so locales_source was empty.
New patch fetches admin/config/regional/language (which includes tabledrag js so db is at least primed) before exporting PO file.
Comment #56
nod_haha awesome. Now we just need to clean it up or are you still seeing the issue with the JS files added during ajax calls?
Comment #57
Wim Leers@larowlan: GREAT job! :)
Final review before marking as RTBC. Looks very good :)
s/ajax/AJAX/
Just beyond col 80?
s/drupal_add_library/drupal_add_library()/
s/so that locales_source/so that the locales_source/
Missing the "Implements hook_library_info()." comment.
s/Drupal specific/Drupal-specific/
I think this is an accidental change? We *don't* want to change this!
Comment #58
Albert Volkman CreditAttribution: Albert Volkman commentedRe-roll with @Wim Leers requested fixes.
Comment #59
larowlanThanks Wim and Albert, good catch on the hidden flag on the testing profile.
Comment #60
tim.plunkettThis looks almost RTBC! Just some clean-up points.
This should be
@return string|false
, and the description should clarify what FALSE signifies. It should also end with a trailing full stopCombine this into one line
This would make sense to have locale_library_info() directly before locale_library_info_alter() in the code.
Comment #61
Albert Volkman CreditAttribution: Albert Volkman commentedThanks @tim.plunkett
Comment #62
Wim LeersFinal review. Three style nitpicks. One actual — yet minor — regression.
We should change this from
['library'] = array(array('foo'))
to['library'][] = array('foo')
.'scope' => 'footer'
was lost here.We should change this from
['library'] = array(array('foo'))
to['library'][] = array('foo')
.We should change this from
['library'] = array(array('foo'))
to['library'][] = array('foo')
.Comment #63
larowlanFixing issues flagged at #62
Comment #64
nod_That is very awesome :)
I'm very tempted to RTBC it :p It'd be great to have that committed this week, this is the most important frontend performance-related patch ever.
Please give credit to SebCorbin first for this patch he did the hard work of making all the library_info declarations.
Comment #65
pounardSebCorbin++ nod_++ RTBC?
Comment #66
nod_And the other people involved++ :)
Just need confirmation the new
drupal_get_request_content_type
function is done correctly, I don't know is that's using the container properly or not. Looks like it but I can't say.Comment #67
pounardI don't like this function, I'm asking myself why is that in the global scope, and will it help other pieces of API than the JS/CSS helpers? It seems to be a wrapper on top of the Request to fetch something that the Request object should probably capable of giving it us in a more direct way, without the need of a wrapper.
Comment #68
nod_can you try it out please?
/me don't even know where to start looking.
Comment #69
pounardRight now, I can't, I'm @work and I don't have a complete environement for this, but I manage to find some time in the weekend I'd try. EDIT: The first place to start is maybe open a support request with the WSCCI tag explaining that you wrote this wrapper function and asking if it exists a better way to fetch this value. I'm thinking that the WSCCI team will probably answer you quickly.
Comment #70
larowlanpounard, are you saying this should be a public method on the request object?
If so I can tackle but won't be for a day or so.
Comment #71
larowlanFwiw I started out without the wrapper but the amount of logic required in the if statement was unwieldly so figured a global wrapper might be useful.
Comment #72
pounardI'm not saying that it must be a method of the Request object, but that there may be a more direct way to obtain it, that's why I propose a support request to the WSCCI team and not a feature request.
Comment #73
Wim LeersI'll try to talk to @Crell about this.
Comment #74
Wim LeersI talked to @Crell. First of all he said that we should *never* use static caching for Symfony stuff.
He told us we should do the following:
But soon, we'll be able to do the following:
So we should add a @todo to make sure we do that clean-up later (but @Crell first has to make that possible first).
Comment #75
nod_All right should be an easy one :) someone reroll?, that'll take it very very close to rtbc :)
Comment #76
SebCorbin CreditAttribution: SebCorbin commentedComment #78
SebCorbin CreditAttribution: SebCorbin commentedDamn, I forgot about the install state...
Comment #79
nod_wow, someone rtbc that stuff, it's the most important patch for JS. Ever. (funnily enough, there is no js changes :p)
Comment #80
pounardRTBC.
Comment #81
Wim LeersRTBC HELL YEAH!
Comment #82
tim.plunkettI've also reviewed this many times and think its the right approach. RTBC++
Comment #83
SebCorbin CreditAttribution: SebCorbin commentedRe-rolled as http://drupalcode.org/project/drupal.git/commitdiff/34e4ef48118d1a4d9155... modified toolbar.module
Also removed unused
$module_path
variable in toolbar.moduleComment #84
tim.plunkettCan you post an interdiff?
Comment #85
SebCorbin CreditAttribution: SebCorbin commented@tim.plunkett here's a re-roll patch with a patch interdiff against #78 (no changes except for the settings that conflicted)
Comment #86
SebCorbin CreditAttribution: SebCorbin commentedAnd here's a new one, with unused
$module_path
removed in toolbar.module, and already stated dependency to jquery.cookie.interdiff is against #85
Comment #87
nod_Reroll ok for me.
Comment #88
nod_Comment #89
sunAwesome patch! I really like it.
I reviewed this patch in-depth and found a couple of issues with it. I did not read comments on this issue, in order to try to stay as ignorant for reviewing the final implementation.
Let's remove the "drupal." prefix from registered libraries. This was introduced, because System module was more or less the only module in core that ships with any libraries and we wanted to separate Drupal stuff in the /misc folder from jQuery and other stuff.
However, in this larger scope the "drupal." prefix doesn't really make sense. I think that we should only use the "drupal." prefix for stuff in the /misc directory. For everything else it is unnecessary.
The new hook_library_info() implementations have been added to the bottom of .module files, whereas it is (not a standard but) common practice to put the "central" info hook implementations to the top.
I do not want to hold up this patch on that, but it would be great if we could quickly adjust that before commit - otherwise, let's do a follow-up patch/issue.
drupal_add_library() resolves nested dependencies already, so I don't quite understand why we're specifying the full stack of dependencies instead of just the "highest" dependency?
Removing the deeper dependencies doesn't affect how this patch works, so I'm happy if we clean this up in a follow-up issue.
'type' should always be set for every JS item. I do not see where and why this would be changed in this patch. Is this additional check for 'type' being set an artifact of earlier debugging or approaches?
This cannot be contained in hook_libraries_info(), since it is incompatible with caching of the registered libraries. hook_libraries_info() is a static declaration/registration of available libraries provided/shipped by modules and not supposed to change on every request.
It looks like this change actually belongs into #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page instead.
The farbtastic library name change is irrelevant for this issue and should be reverted. I don't think our code is actually the code of that Google code repo (development on that seems to have stopped in 2009/2010), although I could be mistaken. In any case, this rename seems out of scope for this issue.
Lazy-injecting the base information for settings in a hook_js_alter() implementation doesn't fly for me. I guess this change is the cause for the
isset($item['type'])
check in locale_js_alter(), which is ultimately going to be only one of many consequences. This handling of defaults has to be carried out by drupal_get_js().This change also seems to belong into #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page instead.
Comment #90
nod_Well some reading could have helped.
1) The drupal. prefix is here for a reason, it's because all this stuff lives under the Drupal object, Drupal.tableDrag, Drupal.ajax and so on. Notice how the html5 shiv doesn't have it. JS will be out of misc and in the system module very soon after this patch we can't rely on that.
jquery.farbtastic, under the $.fn.farbtastic, same thing.
2) bottom of files, fair enough, I'd rather follow up issues, it's a lot of files.
3) No, explicit dependencies, the actual script is using and the jQuery object and the Drupal object so it should depend on both even if drupal.settings depends on both as well, otherwise it'll be impossible to build AMD/commonjs ever and will make things much harder to take apart in contrib. There is nothing to clean-up here it's the whole point of the patch.
4) 'type', indeed it comes from the _js_alter() but this is needed since drupal_add_js does way too much, the other issue about js on every pages comes from drupal_add_js doing way too much and putting defaults everywhere. So well, if you have a better idea you're welcome.
5) About caching, maybe. Still works for me™ I can put that with the rest of the ajaxPageState in drupal_get_js if that suits you better. even if it's an empty array() this library definition needs to stay.
6) the js_alter hook, well, couldn't make it work another way If you have a better solution feel free to update the patch.
This patch is aimed at simplifying how we process js, the _js_alter, the wierd things in settings will be gone once we make that work #1751602: Use Assetic to package JS and CSS files but this patch needs to get in before we can start on that. So yeah, not great but works really well.
Comment #91
nod_Changed the settings thing, and removed the isset from local.module, let's see what the testbot says.
Comment #93
nod_Tried a few other things: can't make it work. I don't know how to do it another way, feel free to fix the patch if it doesn't suit you, there are several others for which it's good enough for now.
re-up #86.
Comment #94
nod_This is holding up a lot of follow-up work, that will eventually lead to fixing the feedback from #89.
Comment #95
nod_grr double post.
Comment #96
catchThis issue is great. I'd really, really like to review it before it goes in though (but can't do so today), so assigning to me. This shouldn't stop webchick or Dries reviewing it too.
Comment #97
sunI understand that you want to move forward. However, the parts about settings that have been outlined in #89 really do not look acceptable to me. We need to find a better solution for that. I don't think that it is proper to treat Drupal.settings as a library -- the amount of additionally required adjustments for them in this patch pretty much prove that settings do not work like a library. It's not really clear to me why those changes are part of this patch in the first place.
Can we just do the files to libraries conversion here?
On the other issues:
That's a very custom interpretation of the server-side library names. I don't see a problem with it, but we need to make sure that this gets documented.
That said, some scripts/libraries may extend a parent object in multiple ways, so I'm relatively sure that it won't always be possible to determine a unique name à la
drupal.foo
orjquery.ui.bar
. I wonder whether the concept will lead to inconsistencies.I do not think that this is the case. drupal_add_library() already resolves all dependencies of dependencies, which only takes a couple lines of code. The very same can be done by a dependency resolving function for AMD/commonjs. If we wanted to, we could resolve and prepare the dependencies of all libraries within drupal_get_library() already.
However, having to manually specify all nested dependencies sounds very cumbersome and prone to errors to me. That's a typical job for computers, not for humans.
Comment #98
nod_Because not all scripts needs Drupal.settings, we have to add it as a library, for now.
If the processing wasn't trying to be too smart and add default settings all over the place when it's not needed (meaning that jquery and drupal get added to all pages all the time, yes that's the reason behind #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page) we wouldn't need it. Then to fix that properly we'll need to change the internal API of this thing, which is going to be a huge mess to clean up, making this big patch a huge patch. Which is why the API change of js processing will be handled in #1751602: Use Assetic to package JS and CSS files issue to clean all that up and hopefully make the settings dependency obsolete.
Show me that the current patch breaks stuff and I'll change the way it's done somehow (I tried, I really tried to make it pretty, I just can't). I don't want to have an API change in this patch that will push it away that much further.
That's not the point. Try out AMD, you'll see the problem with what you're saying right away, read the AMD issue, look at the AMD sandbox. If you need to access "it" from your JS code, you need to declare "it" as a dependency, regardless of the rest of if it's a dependency of something else you're using.
By all means, show me a reproducible way of how this badly breaks or improve the patch to make it better while keeping the same API. Until then I really can't do much more than push for this exact patch to get in so that we can get on with the rest of the clean-up.
Comment #99
Wim LeersComment #100
catchI agree that settings feels like a genuine library, it depends on libraries, libraries depend on it.
Ouch, but it has a @todo.
Do we want to deprecate both this, and direct usage of drupal_add_js(), and then remove this again? I'm wondering if there should be a follow-up patch to make it _drupal_add_js() to indicate it's an internal function. Also wondering whether anything in core is actually using #attached['js'] now but I assume that needs to stay for external js or similar.
Comment #101
moshe weitzman CreditAttribution: moshe weitzman commentedYes, I would love to rename to _drupal_add_js and make it internal only ... We do use #attached['js'] and AFAIK, it remains the preferred way to add js to the page or else you break render cache. Eventually, we should deprecate drupal_add_library() and drupal_add_css() as well (unless SCOTCH changes everything).
Comment #102
nod_So we asked crell about this piece of code, he told us to put the todo since we can't currently do that but it'll be possible to make it shorter soon, once some patches gets in, so… blame Crell :)
Yep, so I don't want to put API changes in this patch, so that's the only way I was able to do that. That's right, core don't use #attach js anywhere.
Yeah there are a few things API-related that will need to be taken care of to clean up the alter thing. Making a new internal function to separate and simplify what drupal_add_js is currently doing, we can actually keep that around to add implicity dependencies on jquery, drupal, and drupal.setting for every script added by drupal_add_js just so you know, we don't horribly break contrib.
I guess we can make a intermediate followup between this issue and the Assetic issue.
Comment #103
Wim LeersThe @todo can only be solved when WSCCI is further along.
Ideally: #attached should be the only way to add/remove CSS/JS/libs. drupal_add_js(), if we keep it, should indeed become a private/internal function. But those are outside of the scope of this patch (as nod_ says: no API changes in this patch).
Besides that, I think the only unaddressed question here is whether it's acceptable to have Drupal.settings be specified as a library. Conceptually, it makes sense, as @catch and I have outlined above. However, @sun makes a good point: part of the concept of libs is also that they're not tied to the current page request. However, I feel that is a minor issue in comparison with the major improvements this patch brings. I think it would be acceptable to create a follow-up issue for that aspect?
So, what is still blocking this patch from going to RTBC status?
Comment #104
cosmicdreams CreditAttribution: cosmicdreams commentedWhat would that follow up be? to create a new construct for page specific libraries?
Comment #105
nod_Follow-up: #1762204: Introduce Assetic compatibility layer for core's internal handling of assets. The summary still needs some work but that's mostly it.
Comment #106
sunMy two major concerns are:
1) The actual settings are not declarative, they are dynamic, by design. Info hooks are not supposed to contain highly dynamic data. In fact, this patch revealed that we're needlessly re-computing all library info on every single page request and missing a cache for hook_library_info() currently. In addition to that, system_js_alter() is an alter hook that injects default properties, which is architecturally wrong.
2) It is not clear how modules are supposed to add settings. It looks like they need to add the drupal.settings library first and then add the settings. This feels odd. Especially if you consider that (further) settings might be added to a page in a different (code) spot than the library.
I'd really prefer to take that settings part out of this patch and discuss it in a separate issue. It's really just that part that bugs me — all of the other js-to-library changes are looking great.
Comment #107
nod_That's fair enough. the library def stays because we need the dependencies but it doesn't add anything else.
Let's see what the testbot says.
Comment #108
sunThanks! RTBC if bot comes back green.
Comment #110
nod_Haha, that really scared me, it's failing because it can't write to disk somehow. I'm asking around to see what's up before retesting.
Comment #111
nod_same as #107, with correcter spelling in comment.
Comment #113
nod_All right had a few issues with the tests, all green (local at least). Hope you have some space now testbot, here is the patch.
Comment #114
Jelle_SJust thinking ahead for documentation etc. Is there still a valid use case for
drupal_add_js
once this patch gets in? Since all contrib modules are encouraged to use Drupal's behaviors (and thus have a dependency to drupal.js), shouldn't they define their script inhook_library_info
as well?Comment #115
Jelle_Scross-post...
Comment #117
nod_#113: core-js-explicit-dependencies-1737148-113.patch queued for re-testing.
Comment #118
Jelle_SOn a sidenote:
Shouldn't we change
drupal_add_js
to dowhen
$options['type']
issetting
so that we can just do this:in stead of:
Just a small change dat improves DX (imho)
Comment #120
Jelle_S1 fail on file unmanagedDelete test... Bot still acting out?
One more retest...
Comment #121
Jelle_S#113: core-js-explicit-dependencies-1737148-113.patch queued for re-testing.
Comment #122
nod_(while waiting for the bot) #118 that'll be doing what drupal_add_js has been doing, implicitly adding things so I'm not really in favor of this.
Comment #123
Jelle_STrue, but it seems to me that when you're adding js settings, you'll be adding js files as well anyway... But I can see your point.
Comment #124
catchUnassigning me since I was able to look at this, looks much better with the request stuff outside the hook_library_info() yes.
Comment #125
nod_from #108: Rock & roll.
Comment #126
Dave ReidThis seems like the type of issue that could use a couple rounds of manual testing, correct?
Comment #127
nod_Well I guess, it's been tested to death by me at least and I'm sure about a couple of other people that tested it as well.
Comment #128
webchickCool, this patch seems like a great start to reducing our mobile JS footprint. Looks like everyone's concerns from above have been addressed as well.
Committed and pushed to 8.x. Thanks!
In general, I agree with manual testing (particularly in 7.x), but while we're in code thaw, as long as basic testing has been done, I think we'll catch any JS regressions pretty fast once patches like this get committed.
This definitely needs a change notice.
Comment #129
nod_There is a change notice at https://drupal.org/node/1764252, feel free to make it better or tell me how to.
I'd like to thank SebCorbin for all his work that lead to this patch, he's been working in the shadows to make core use AMD (and he succeeded! Check out the AMD sandbox).
I've never seen that many people in one of my JS-related issue, thanks to all of you :)
Now, go enjoy the speed (makes a lot more difference without toolbar activated)!
Comment #130
webchickAwesome, thanks for the change notice! And yay for faster Drupal. :)
Comment #131
Mark Theunissen CreditAttribution: Mark Theunissen commentedI went to read the change description but I don't know what AMD is. Perhaps a clearer explanation of that this means:
"This change will make possible the use of core JS in the context of AMD by contrib."
Otherwise, great work!!
Comment #132
SebCorbin CreditAttribution: SebCorbin commentedAMD : Asynchronous Module Definition
More info http://github.com/amdjs/amdjs-api/wiki/AMD and #1542344: Use AMD for JS architecture
Now Drupal will also be modular in the front-end :)
Comment #133
tim.plunkettOpened #1764634: Drupal.settings is only populated if a custom setting is added as a major follow-up.
Comment #134
sunFollow-up bug: #1776030: Machine name JavaScript behavior is broken, since jquery.once isn't loaded
Comment #135
c960657 CreditAttribution: c960657 commentedFollow-up: This introduced a regression in the OpenID module. This is included in the latest patch for #1538462: Cannot log in with OpenID due to "required" attribute (another issue blocking usage of OpenID).
Comment #136
Dave ReidI'm curious if anyone had tested how this works with themes that use scripts[] = myfile.js and the fact that themes cannot implement basic hooks (so no hook_libraries). I mean I guess a theme can use themename_libraries_alter() but this is adding extra complexity for users that normally don't need to touch PHP files. Is there a follow-up for this at all?
Comment #137
nod_maybe not a direct follow-up but very related #1762204: Introduce Assetic compatibility layer for core's internal handling of assets where this'll turn drupal_add_js into a legacy function adding drupal.js, jquery.once, drupalSettings and jQuery as dependency for the script. Then it's just a matter of making scripts[] use drupal_add_js to add files.
Comment #138
sunFollow-up: #1787758: drupal_get_library() / hook_library_info() is not cached
Comment #140
larowlanAdded followup for the @todo we added #1941288: Regression: ajaxPageState not being updated with AjaxResponse, assets (js/css) being added twice
Comment #141
xjmUntagging. Please remove the "Needs change notification" tag when the change notice task is complete.
Comment #141.0
xjmdrupal_add_js in color module
Comment #142
penyaskitoLanded to the change record by google search, and edited it for using render arrays.
Please verify this is correct:
https://drupal.org/node/1764252/revisions/view/2735445/6898303