We are deprecating underscore as an API core provides, and limiting its use to within modules with Backbone integrations. It is already marked internal, and the library has been renamed to internal.underscore (Backbone has also internal and will be eventually deprecated.)
Underscore API documentation:
https://underscorejs.org/
The only two non-deprecated uses of underscore in core are in the following libraries:
drupal.editor.admin:
version: VERSION
js:
js/editor.admin.js: {}
dependencies:
- core/jquery
- core/once
- core/jquery.once.bc
- core/drupal
- core/internal.underscoredrupal.filter.filter_html.admin:
version: VERSION
js:
filter.filter_html.admin.js: {}
dependencies:
- core/jquery
- core/once
- core/jquery.once.bc
- core/internal.underscore
keys -> Object.keys
omit -> Object.keys + Array.prototype.forEach to build a new object
has -> Object.prototype.hasOwnPropertyOf
reduce -> Array.prototype.reduce or use Object.keys + Array.prototype.forEach
indexOf -> String.prototype.IndexOf
each -> Array.prototype.forEach
every -> Array.prototype.every
some -> Array.prototype.some
pluck -> Array.prototype.map + object destructuring
union -> use object destructuring to return a combined object
isEmpty -> compare Object.keys length
intersection -> Array.prototype.filter based on Array.prototype.includes on the second array
Needs replacement pattern: difference
Replace the use of underscore APIs in the above two libraries with vanilla JavaScript, and remove underscore from the respective library definitions.
TBD
N/A
The drupal.editor.admin and drupal.filter.filter_html.admin libraries no longer depend on underscore.
N/A
The drupal.editor.admin and drupal.filter.filter_html.admin libraries no longer depend on underscore.
| Comment | File | Size | Author |
|---|---|---|---|
| #32 | 3270395-31-94x.patch | 30.25 KB | bnjmnm |
| #31 | 3270395-31-10x.patch | 28.32 KB | bnjmnm |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
xjmComment #3
mradcliffeI added some details about what functions need to be replaced in those files.
difference, intersection and isEmpty (for objects) are probably the most complex to code for.
Comment #4
mradcliffeI'll create an initial issue fork for 9.4.x and work on it for a bit.
Comment #5
mradcliffeSome of the underscore usage of _.every, _.pluck, _.some, _.omit are hard for me to understand exactly what's going on, but I think I was able to get it right. The underscore (or lodash) documentation isn't very clear or verbose for optional arguments.
We really need to add api documentation for some of the objects used in this file.
I'm going to stop working on this for now.
Comment #7
mradcliffeI forgot to update the status to Needs work.
Comment #8
xjmComment #10
murilohp commentedRegarding the
differencemethod, during the development I used You-Dont-Need-Lodash-Underscore as a reference for it, so you'll see a combination ofreduceandfilter. I'm moving this to NR to see what you guys think about the solution.Comment #11
xjmNice work!
Comment #12
nod_couple of things, IE11 compatibility issue, and adding a local helper for difference method
Comment #13
murilohp commentedThanks for the review @nod_!.
Comment #14
murilohp commentedSorry for my delay, when you comment just on the MR, I don't get any notification on my dashboard.
Comment #15
xjmCommenting for dashboard notification. ;)
Comment #16
murilohp commentedThanks @xjm!
Comment #17
murilohp commentedRegarding the CR, recently I created Underscore library is deprecated, do you think a new CR is necessary here?
Comment #18
nod_We're missing some test coverage here, some of the replacements are just not functional.
Been at it for a couple of hours but I can't find how to trigger the paths that have problems. I looked at contrib and this whole filter_html api doesn't seems to be used at all outside ckeditor 4 and that is going away. Gives a strong "dead code" vibe here.
CKE5 bypasses all of that and does the work on the PHP side so it's not even going to be used afterwards.
Comment #19
murilohp commentedSo should we write some new tests for both files here? And which replacements are just not functional?
Just for you know, I focused only on
filter.filter_html.admin.js, and looking at thecore/modules/filter/src/Plugin/Filter/FilterHtml.phpfile, you can see thefilter/drupal.filter.filter_html.adminlibrary being attached, so my idea was to move into/admin/config/content/formats/manage/full_htmland test there using CKE4.That's really sad, I spent a lot of time trying to replace the underscore usage there, but that's fine, it's good to know that CKE5 will be able handle this. Testing here with CKE5, I can confirm, the filter_html is not being used, it works with CKE4 only.
Comment #20
xjm@murilohp It's still important to provide clean replacements even for CKEditor 4 integration code, because we need to continue to support CKEditor 4 until the end of 2023 in Drupal 9 and in contrib for the Drupal 10 upgrade path. So it's still totally worth it; we need it for nearly two years. :)
It does mean we could potentially open a followup to deprecate it and remove it from D10.
Comment #21
nod_Fixed most of the issues with the replacement. The difference function was not working as intended (it removed the keys of the object and ended up breaking the generated html filter settings).
Comment #22
murilohp commented@nod_ do you have any suggestions to properly replace the
_.difference? It's really weird to know the function is actually breaking, at least on the/admin/config/content/formats/manage/full_htmlpage, I printed all the objects before and after changing the difference function, and the results were the same. Maybe I was just missing something.We'll need tests to cover this right? You just removed the underscore dependency from
core/modules/filter/filter.filter_html.admin.es6.js, the code is still calling _.difference and somehow all the tests passed.Comment #23
xjmI guess this is NR now?
Can someone check for resolved threads and either mark them resolved or put in a thumbs-up emoji so that I can do so with my committer powers? :)
Comment #24
nod_Still one function left to replace
Comment #25
nod_ok, put the code back as it was mostly. The error I ran into is present in the underscore code as well, so it's actually working as expected :)
To reproduce the error with backbone and the replacement:
<kbd><0 href hreflang> <1> <2> <3> <4 cite> <5> <6 type> <7 start type> <8> <9> <10> <11> <12 id> <13 id> <14 id> <15 id> <16 id> <17 src alt data-entity-type data-entity-uuid> <18> <strong> <em> <a href> <ul> <li> <ol> <blockquote> <img src alt data-entity-type data-entity-uuid> <u>Can someone open a follow up with those steps to fix the issue?
The replacement is actually doing the exact same thing so +1 from me. I'd say it's RTBC now.
Comment #26
murilohp commentedI had the same issue locally @_nod, I thought I was doing something wrong. Anyway the followup is created: #3275054: It's not possible to drag an item from CKE and change the "Allowed HTML tags" on a text format
I've also updated the code to fix the CS errors
Comment #27
murilohp commentedRe: #20
Thanks for the explanation @xjm! I've created a followup issue to remove the files on D10, #3275080: Remove unused javascript from filter module.
Comment #28
wim leersI ended up simply +1'ing all (two) of @bnjmnm's remarks 🙈 😅
P.S.: I think all of this JS will disappear once CKEditor 4 is removed from Drupal 10. CKEditor 5 does not rely on this JS to update the
filter_htmlconfiguration. It was the only way to make this work at the time of CKEditor 4, because it was technically impossible to have metadata about which HTML elements (tag, attributes, attribute values) a specific CKEditor 4 feature supported. Which meant we needed a hidden CKEditor 4 instance in the admin UI to get that metadata live, instead of declaratively (in a YAML file/annotation). With CKEditor 5, that did become possible, and so it's been implemented that way. It's much more thoroughly tested.Therefore I think the follow-up that @nod_ is asking for in #25 may not be necessary. This JS will simply no longer be used. And at the time it was written, it was not possible to execute JS in tests for Drupal core. Writing such tests for it today would not only be a nightmare, but also pointless (because it is no longer used).
Comment #29
nod_added the dep.
Technically the whole thing is a public api. I haven't found a use in contrib so probably safe to remove it, but it should be deprecated at least no?
Comment #30
wim leersYep — but definitely out of scope here!
Comment #31
bnjmnmComment #32
bnjmnmComment #33
bnjmnmComment #36
bnjmnmCommitted to 9.4.x and 10.0.x. Change record published. I haven't seen @murilohp in an issue I've worked on before - so thank you in particular for your work on this!
Comment #38
xjm