Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
In #1149866: Add Backbone.js and Underscore.js to core, the Backbone and Underscore libraries were added to Drupal with references to the uncompressed development versions. Before the release of Drupal 8, we need to switch the references to the minified and compressed production versions of these libraries.
Optionally update to the latest releases as well. Backbone should have a 1.0 release before D8 is released and it would be great to ship with this stable version.
Comment | File | Size | Author |
---|---|---|---|
#23 | interdiff-1800022-21-23.txt | 1.16 KB | herom |
#23 | backbone_upgrade-1800022-23.patch | 139.3 KB | herom |
#21 | backbone_upgrade-1800022-21.patch | 138.2 KB | Wim Leers |
#18 | interdiff.txt | 726 bytes | Wim Leers |
#18 | backbone_upgrade-1800022-18.patch | 150.79 KB | Wim Leers |
Comments
Comment #1
jessebeach CreditAttribution: jessebeach commentedBackbone now has a 1.0 release.
http://backbonejs.org/
We are approaching code freeze. I'm de-postponing this issue and marking it active. We need to get Backbone upgraded and the production version in place before code freeze.
Comment #2
JohnAlbinWe MUST get rid of core/misc. I'm moving 3rd party assets over to core/assets/vendor in #1663622: Change directory structure for JavaScript files
Here's a patch that upgrades backbone.js to 1.0.0 and uses the backbone.min.js in the library definition. It also uses a .gitignore file we have for ckeditor to strip out the docs, examples and tests out of the backbone 1.0.0 downloadable distribution.
Comment #3
JohnAlbinForgot to update the .jshintignore file
Comment #5
JohnAlbinFatal error in Drupal\rest\Tests\Views\StyleSerializerTest->testPreview()
Huh? Retesting.
Comment #6
JohnAlbin#3: 1800022-3-backbone-upgrade.patch queued for re-testing.
Comment #7
Wim Leers@JohnAlbin: you can't just upgrade Backbone; you'll have to update all JS in core to make it compatible. Most will be easy, but Edit's won't be. Also, this should wait for #1678002: Edit should provide a usable entity-level toolbar for saving fields.
I agree we must upgrade to 1.0 of course :)
Comment #8
JohnAlbinAhhh… oh, well. :-)
Comment #10
nod_Can I haz upgrade? Blocking #2137235: Make core JS work with a subset of jQuery
Comment #11
jessebeach CreditAttribution: jessebeach commentedI won't be able to get to this in the coming week.
I've been backporting Toolbar to Navbar, which is running on Backbone 1.0. So far I've only found one upgrade issue: #2139571: underscore.js fails to compare DOM nodes correctly; Toolbar JS will fail when upgraded to Backbone 1.0.0.
wimleers is currently backporting D8 Edit to D7. Those two modules are the ones using Backbone most extensively. Once he's had a chance to do that backport, we'll know what issues we'll have with an upgrade.
Comment #12
Wim LeersOnly in
contextual.module
andedit.module
, invasive changes were necessary. Contextual.module should be tested very thoroughly. Edit.module too, but to a lesser degree, because it's a straight port from the changes that were made in the D7 backport of Edit to support Backbone 1.x.This is built on top of #2159965: Fix two memory leaks + subtle bugs in Edit's JS (discovered by working on the Backbone upgrade in the D7 backport), so marking postponed.
Note that this does NOT bring the "production" version of Backbone/Underscore, because they (appropriately) reference a source map file (supported by Google Chrome: makes it easy to debug minified JS) and ideally we'd include those in Drupal core. That's scope creep here, so let's just focus on making the upgrade to Backbone 1.1 here and then worry about the minified version later.
Comment #13
jessebeach CreditAttribution: jessebeach commentedI think that, in the code above
should be
Comment #14
jessebeach CreditAttribution: jessebeach commentedActually, strike that comment in #13. On further inspection, the intent is to bind the
listenTo
to the model, not the view. The event handler for this line refers to a method on the model, not the view.BUT, there might be some extraneous code here. We don't need to bind the event handler to the object through an argument, which is what the fourth argument in these two lines would suggest:
With
listenTo
, the handler is bound to the object that invokeslistenTo
.Honestly, that quote from the docs is....damn muddled. Which object now? The object invoking
listenTo
or the one raising the event? Let's check the code in Backbone...So, the pertinent line is
where
implementation
ison
andthis
is the object that invokedlistenTo
.The fourth argument passed to
listenTo
in the code changes above is never referenced in Backbone and just silently passes into the æther.Comment #15
Wim Leers#14: So stupid that I did that conversion incompletely. I went through the exact same sequence of "wtf are these docs saying now?" to just looking at the code and coming to the same conclusion. (I even commented about that on the somewhat-authoritative StackOverflow post about this: http://stackoverflow.com/questions/16823746/backbone-js-listento-vs-on#c....)
Here's a pull request to clarify that: https://github.com/jashkenas/backbone/pull/2935.
Comment #16
webchickThat got committed, so back to "needs review."
Comment #17
jessebeach CreditAttribution: jessebeach commentedThere's one more instance in
EntityModel.js
. I searched with the following regex:Comment #18
Wim LeersGood catch!
Comment #20
Wim LeersArgh, I managed to include the patch for #2159965: Fix two memory leaks + subtle bugs in Edit's JS (discovered by working on the Backbone upgrade in the D7 backport) (that landed already) in this patch, hence it of course does not apply. Stupid stupid stupid.
Comment #21
Wim LeersHopefully less braindead.
Comment #23
herom CreditAttribution: herom commentedcore/modules/edit/js/models/BaseModel.js
was lost between #18 and #21. adding back.Comment #24
jessebeach CreditAttribution: jessebeach commentedAlright, this looks good! If this is going to create any issues (I don't think it will), I'd rather know them sooner rather than later.
Comment #25
webchickLet's get some manual testing on this first. Navbar, Edit, and contextual links should all be clicked around in a few browsers to make sure they still work.
Comment #26
Wim Leers#25: agreed. I already did that, but somebody else should do that too, to make sure I didn't miss anything.
Comment #27
herom CreditAttribution: herom commentedI tested this patch, and compared with a clean install, on firefox and chrome.
everything I checked was identical (including those mentioned in #25).
Comment #28
catchThanks for testing. Committed/pushed to 8.x, thanks!
Comment #29
nod_Same here, looks like everything working fine. RTBC+1Too late, anyway :p
Comment #30
Wim LeersThat was fast — thanks all!