This issue is used for tracking progress on cleaning up core javascript.
Problems
- code is inconsistent,
- code is not robust enough to be used with bad 3rd party code (extending prototypes),
- there are flaws in the design of a number of components,
- jQuery selectors are abused,
- there is no proper place for javascript files in code folder structure,
- javascript coding standard are incomplete #1778828: [policy, no patch] Update JS coding standards.
Requirements
- Core JS should pass JSHint without errors using: #1664940: [Policy, patch] Decide on JSHint configuration
- take advantage of IE8 javascript features
Issues
javascript clean-up
- #1574470: Selectors clean-up,
- #1993322: [Meta] Drop IE8 support,
- #1440628: [Meta] javascript toolbar/tableheader with url fragment mess,
- #1663622: Change directory structure for JavaScript files,
- name all constructors with initial uppercase character (might be the case, haven't checked everything) and more importantly remove the uppercase if it's not a constructor
jQuery clean-up
- #2153177: Convert type selectors to be compatible with jQuery native-API selector,
- #822128: "Textarea + summary" widget broken when field allows multiple values (followup) and associated JavaScript uses fragile selectors
- #1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors
Done
- #1754344: Rename Drupal global JS object to drupal
- #1460022: Prefix all jQuery variables with $
- #1420706: Access DOM directly instead of attr('xx')
- #1974580: Introduce domReady to remove jQuery dependency from drupal.js and clean it up.
- #1719640: Use 'button' element instead of empty links
- #1507960: [meta] Isolate and/or remove IE-specific hacks in core markup, CSS and JavaScript
- #1745964: JSHint: Remove unused variable declarations
- #1483396: [policy, no patch] JavaScript coding standards for variable declarations
- #1751008: filter inside .prev() or .next() and not right after
- #1737148: Explicitly declare all JS dependencies, don't use drupal_add_js
- #1481560: Add "use strict" to all core JavaScript to enforce clean code,
- #1420022: Remove dead code: progress.upload_callback, progress.error_callback
- #1428534: Use === and !==,
- #1428524: Replace all $.each() with filtered for loop (I'm not talking about
$(selector).each()
which makes total sense to use), - #1089300: Clean up drupal.js,
- #1518012: Every additional function from Array.prototype called on ajax requests.
- #1459164: use test() to match string for BOOL comparison
- #1651270: Remove IE8-specific workaround for lack of CSS3 box-shadow support
- #1419968: Replace $('selector', domelement) with $(domelement).find('selector')
- #1428538: Use jQuery toggle
- #642734: Simpletest behaviors should process the elements only once
- #1749782: Use event.preventDefault(); and event.stopPropagation(); instead of return false;
I'm not sure it all applies, it's the kind of things I saw in contrib. and I suppose most of them can be found in core.
Benefits
Safer and faster execution of core JS (selectors simplification can go a long way here). It'll make refactoring go smoother too (one day, when file-level closures go away).
And having a strict code style literately fixes bugs #784670: autocomplete.js can iterate over functions in Array objects.
The first patch going this way just got commited to D8 \o/ #1400310: Use of .parents() is really .closest().
How does this fit with other initiatives?
We are in touch and share some issues with the mobile and html5 initiative.
How can I help?
If you are subscribing to this issue and want to help, note that there is a very long list of issues linked from the opening post that are dying out for reviews, re-rolls or initial patches so it should be easy enough to find somewhere to start.
If after looking at that list you don't know where to start, grab nod_, catch on IRC (#drupal-contribute) or just ask around, and we'll try to help you find something.
Comments
Comment #1
ksenzeeDiscussed this some on IRC and everything currently in the issue description sounds great to me, and I think (I hope) pretty unobjectionable.
Comment #2
gary4gar CreditAttribution: gary4gar commentedComment #3
tstoeckler@gary4gar: Please don't post subscribe comments anymore. We have a big green button for that at the top now :)
Comment #4
gary4gar CreditAttribution: gary4gar commentedI have found eval on line 363 of ajax.js,
var progressBar = new Drupal.progressBar('ajax-progress-' + this.element.id, eval(this.progress.update_callback), this.progress.method, eval(this.progress.error_callback));
Now,so have two questions
1) how do we remove eval here? I think we can pass these as anonymous functions because functions are first-class citizens in javascript.
2) the reason I am asking this instead of doing it myself is because I have no way to try it out. so more importantly, How do test if changes I make in Drupal's javascript works or not?
Please, excuse my noobisness. I am quite new to drupal.
@tstoeckler
I have removed my comment & would use the follow button from next time :)
Comment #4.0
gary4gar CreditAttribution: gary4gar commentedAvoid some bikeshedding
Comment #5
nod_hey gary4gar, I was going to use this issue for meta talk, each dot in the list should/will get it's own issue I made one for
eval()
removal. If you can't test it, don't change it. Someone else will be able to and will certainly help you out on the testing protocol.if anyone wants to take up another task just create a new issue and link to it in here, i'll update the list.
Comment #5.0
nod_link issue
Comment #5.1
nod_add jquery task
Comment #5.2
nod_add a related list
Comment #5.3
nod_messed up previous edit
Comment #6
nod_I'd like to add replacing
$('selector', domContext)
with$(domContext).find('selector')
too.The first one is expanded to the second internally by jQuery, it saves a function call and makes it more readable, it's closer to how we're actually traversing the DOM tree. I just want to check we're agreeing before adding to the todo list.
Comment #7
nod_Ended up just doing it, please help review this big big patch #1419968: Replace $('selector', domelement) with $(domelement).find('selector').
Comment #7.0
nod_Updated issue summary.
Comment #7.1
nod_new task
Comment #7.2
nod_add ajax.js issue
Comment #8
nod_as suggested by sun, adding new tag.
Comment #9
droplet CreditAttribution: droplet commented2 more
#1, #1420048: fix menu* JS indention
#2, #1420056: moving shortcut local var
Comment #9.0
droplet CreditAttribution: droplet commentedwrong issue number
Comment #9.1
nod_move selector away from main list
Comment #9.2
nod_add the selector issue back
Comment #9.3
nod_link to issues
Comment #10
nod_Added a couple of issues, no patch for all of them yet so feel free to take care of some if you're in the mood.
Comment #10.0
nod_link to new issue
Comment #10.1
nod_add new issues
Comment #11
droplet CreditAttribution: droplet commented#1428534: Use scrict equals,
#1460022: Prefix all jQuery variables with $
I'm thinking .... if split them into each module a issue thread that may work more efficiently ?
we can fix all performance and standard problems.
Comment #12
nod_What I got from irc when I asked was that we do 1 small patch on 1 file, get that review, get that committed and followup with the big patch changing everything.
And I wouldn't start on the $ patch just yet, wait a week or two first :D
Comment #13
nod_Actually, here is your answer #1530652: [META] Switch from == to === to prevent mistakes and make brute force password attacks harder :)
Comment #13.0
nod_use strict
Comment #14
nod_updated summary
Comment #15
sunI'm missing an issue for replacing context with $context.
I'm passing $context to custom behaviors in Dreditor, and I do not understand why we're passing the native DOM context instead of a jQuery $context in core.
Comment #15.0
sunbetter presentation
Comment #15.1
nod_add selectors clean-up
Comment #16
klonos...coming from #1332066: [Meta] Update / Refactor JavaScript as a result of upgrading to jQuery 1.7.
Comment #17
nod_Let's just hope we don't have to make it to winter.
The big table in the issue summary needs to be full of issues, ping me on irc or mail if you need help getting started with something. There are quite easy clean-ups to be taken care of for people starting with JS :)
Comment #17.0
nod_Add oversized table
Comment #17.1
nod_add ie8 css/js issues
Comment #17.2
nod_issues in table
Comment #17.3
nod_jshint to table
Comment #18
betoscopioHi nod_, i'm interested on those easy JS clean-ups, can be a good start to dive into this issue. There is a tag that flag this kind of issues?
Comment #19
nod_oh yes, actually there is a tag: js-novice or you can also help review the JavaScript clean-up queue.
Ping me on IRC if you need some help :)
Comment #19.0
nod_jshint
Comment #19.1
nod_js folder
Comment #19.2
nod_add jshint cleanup issues
Comment #20
nod_Huge update on the JSHint issues. Most of them are either verified, RTBC or with a patch adressing most of the warnings.
Comment #20.0
nod_file removed
Comment #20.1
nod_promote jshint config
Comment #20.2
nod_Add some refactor issues
Comment #20.3
nod_autocomplete issue for D8
Comment #20.4
nod_shortcut issue
Comment #20.5
nod_add statistics issue
Comment #20.6
nod_Change first contact to me for IRC help
Comment #20.7
nod_Remove issue from list that is already in the table
Comment #20.8
nod_Reorder issue lists for clearer view of what's left
Comment #20.9
nod_add machine name refactor issue
Comment #20.10
nod_toolbar issue
Comment #21
pascalduez CreditAttribution: pascalduez commentedCreated the #1749782: Use event.preventDefault(); and event.stopPropagation(); instead of return false; issue.
Comment #22
nod_update the summary :)
Comment #22.0
nod_add undeclared var issue
Comment #23
Jelle_SCreated the #1751008: filter inside .prev() or .next() and not right after issue.
Comment #23.0
Jelle_SAdded new issue #1749782
Comment #23.1
Jelle_SUpdated issue summary.
Comment #23.2
Jelle_SUpdated issue summary.
Comment #23.3
Jelle_SUpdated issue summary.
Comment #23.4
Jelle_SUpdated issue summary.
Comment #23.5
pascalduez CreditAttribution: pascalduez commentedAdded more js-cleaning issues to the table
Comment #23.6
pascalduez CreditAttribution: pascalduez commentedTypo
Comment #23.7
pascalduez CreditAttribution: pascalduez commentedAdded more selectors cleaning issues.
Comment #23.8
pascalduez CreditAttribution: pascalduez commentedAdded more selectors cleaning issues.
Comment #23.9
nod_right issue for system.js
Comment #23.10
seutje CreditAttribution: seutje commentedmoved #1749782: Use event.preventDefault(); and event.stopPropagation(); instead of return false; to the done list
Comment #23.11
nod_add couple issues
Comment #24
nod_hi there, can the people working on the selector/refactor issues have a look at this issue #1342198: Use .on and .off instead of .bind, .unbind and .delegate and take what's relevant for them? cosmicdreams did a great job of rerolling the patch with so many changes that got in lately. It'll help keep things revieable, thanks!
Comment #25
Kiphaas7 CreditAttribution: Kiphaas7 commented*shameless plug*
#1763812: [META] Provide complete attach/detach with basic events
Comment #25.0
Kiphaas7 CreditAttribution: Kiphaas7 commentedbetter spot
Comment #25.1
nod_move dependency patch to done
Comment #26
Kiphaas7 CreditAttribution: Kiphaas7 commentedRegarding #25: nod_ indicated that he rather see this added to individual issues than yet another meta issue. So just as an FYI if I start updating issue summaries.
Comment #27
nod_Almost there :)
Comment #27.0
nod_Add tableheader follow-up.
Comment #28
nod_Added dropbutton and tableresponsive to the table.
Comment #28.0
nod_add dropbutton and tableresponsive
Comment #28.1
nod_add progress improvement issue.
Comment #29
nod_Views is in core, so is it's JS. needs the follow-up issues created. There are issues with the jshint validation on the views files.
Comment #29.0
nod_Add views JS
Comment #29.1
rballou CreditAttribution: rballou commentedAdded views JS issue numbers for JSHint
Comment #29.2
nod_add matchmedia
Comment #29.3
rballou CreditAttribution: rballou commentedAdded issue number for final views JS file
Comment #29.4
nod_add clean-up of base.js
Comment #29.5
nod_add views-admin.js
Comment #30
nod_Updated the table with all the JS stuff that got in since last time (ckeditor, edit, editor, tour, seven, picture, translation, translation_entity, path, debounce.js, dialog.js)
And yeah, I just skipped winter because winter is no fun.
Comment #31
droplet CreditAttribution: droplet commentedabout the debounce fucntion:
#1889394: Choose one JS debounce function
Comment #31.0
droplet CreditAttribution: droplet commentedBig update
Comment #31.1
nod_add tour issue
Comment #32
nod_Updated the jshint issues, a few regressions.
Comment #32.0
nod_updated jshint issues.
Comment #32.1
nod_add couple issues
Comment #32.2
nod_move done issues to proper place
Comment #33
nod_In one month we're back to summer clean-up :)
Because of JSHint config changes #1995996: Update JSHint configuration a few JSHint issues will be reopened. Don't forget to update JSHint to the latest version.
Comment #33.0
nod_update issues, add drop IE8 meta
Comment #34
klonos...in 10 days actually :D
Comment #35
hass CreditAttribution: hass commentedExists there any case that moves all
.js
files in modules to/js
subfolder of the modules or this not planed? I've seen it only for CSS files and modules are very inconsistent.Comment #36
nod_#1663622: Change directory structure for JavaScript files
#1298304: [Meta] Move all externally-developed files to core/vendor/{vendorname}/ directories
#1473066: [Meta] Move third-party assets out of core/misc folder
Comment #37
hass CreditAttribution: hass commented#1663622: Change directory structure for JavaScript files is only about
/misc
folder and introduces another inconsistency named/assets
.I'm more asking about toolbar.js, block.js and others located in module folders.
Comment #38
nod_Feel free to open an issue about it. I agree but I haven't tried enforcing that yet.
Comment #38.0
nod_Add a few JSHint issues and
Comment #39
nod_Core passes JSHint validation!
Comment #39.0
nod_close jshint column
Comment #40
nod_Alas, still not done.
I guess it's time to take a serious look at the coding standards. Can people have a look at #1778828: [policy, no patch] Update JS coding standards and help out there? thanks.
Comment #41
klonos[#7230352] ???
Comment #42
nod_Oups, fixed :)
Comment #43
LewisNyman CreditAttribution: LewisNyman commentedComment #44
xjmSo this issue mostly seems to be a tracking issue for a variety of JS work. As normal tasks, the child issues all need to be evaluated on whether they're still doable during the beta, and if appropriate, postponed to a later version. Thanks!
Comment #45
nod_Comment #46
eiriksmAs requested by @nod_: For reference, here is a super minor refactor in block.js: #2503981: Move checkEmptyRegions to named function and before first use (block.js)
Not sure where this fits in the issue description, but here it is for reference at least :)
Comment #49
nod_This issue served me well, it's time to retire it. Our code was improved as much as possible for D8.
Post-D8 we're going for even crazier changes made possible by this huge clean-up. Thank you everyone that participated in all related tasks! See you in the ES6 issue #2809281: Use ES6 for core JavaScript development :)