Jump to:
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | javascript |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | active |
| Issue tags: | JavaScript, JavaScript clean-up |
Issue Summary
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.
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: [Meta] selectors clean-up,
- #1993322: [Meta] Drop IE8 support,
- #1440628: [Meta] the big javascript toolbar/tableheader/overlay/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
- #1460022: Prefix all jQuery variables with $
- #822128: "Textarea + summary" widget broken when field allows multiple values (followup) and associated JavaScript uses fragile selectors
- #1420706: Access DOM directly instead of attr('xx')
- #1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors
Clean-up related bugs/tasks
Done
- #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
#1
Discussed this some on IRC and everything currently in the issue description sounds great to me, and I think (I hope) pretty unobjectionable.
#2
#3
@gary4gar: Please don't post subscribe comments anymore. We have a big green button for that at the top now :)
#4
I 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 :)
#5
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.
#6
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.
#7
Ended up just doing it, please help review this big big patch #1419968: Replace $('selector', domelement) with $(domelement).find('selector').
#8
as suggested by sun, adding new tag.
#9
2 more
#1, #1420048: fix menu* JS indention
#2, #1420056: moving shortcut local var
#10
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.
#11
#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.
#12
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
#13
Actually, here is your answer #1530652: Switch from == to === to prevent mistakes and make brute force password attacks harder :)
#14
updated summary
#15
I'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.
#16
...coming from #1332066: [Meta] Update / Refactor JavaScript as a result of upgrading to jQuery 1.7.
#17
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 :)
#18
Hi 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?
#19
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 :)
#20
Huge update on the JSHint issues. Most of them are either verified, RTBC or with a patch adressing most of the warnings.
#21
Created the #1749782: Use event.preventDefault(); and event.stopPropagation(); instead of return false; issue.
#22
update the summary :)
#23
Created the #1751008: filter inside .prev() or .next() and not right after issue.
#24
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!
#25
*shameless plug*
#1763812: [META] Provide complete attach/detach with basic events
#26
Regarding #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.
#27
Almost there :)
#28
Added dropbutton and tableresponsive to the table.
#29
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.
#30
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.
#31
about the debounce fucntion:
#1889394: Choose one JS debounce function
#32
Updated the jshint issues, a few regressions.
#33
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.
#34
...in 10 days actually :D