Only initialize backbone once #3994
Conversation
|
@rullzer, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bartv2, @LukasReschke and @MorrisJobke to be potential reviewers. |
core/js/oc-backbone.js
Outdated
| /* global Backbone */ | ||
| if(!_.isUndefined(Backbone)) { | ||
| OC.Backbone = Backbone.noConflict(); | ||
| Backbone = OC.Backbone; |
There was a problem hiding this comment.
I'm not sure about the impact of this change. Could this cause problems in apps that load their own Backbone? Why do we restore it here?
There was a problem hiding this comment.
See rulers comment:
Best solution is to always use OC.Backbone instead of Backbone directly. But this fix makes it work with Backbone again as well.
There was a problem hiding this comment.
I read it. I would only use OC.Backbone if one explicitly wants to use it. For example in the Mail app, we ship our own version.
In any case, this will probably work 👍
There was a problem hiding this comment.
Mmmm could be...
Set a breakpoint at https://github.com/nextcloud/server/blob/master/core/vendor/backbone/backbone.js#L56
And see it being called twice. First time root.Backbone will be set to undefined. Second time it is reset.
But yeah always using OC.Backbone is probabaly better. Or I could even add a check...
There was a problem hiding this comment.
This might be cleaner now...
rullzer
force-pushed
the
only_init_backbone_once
branch
from
5780631 to
8f09dab
Compare
core/js/oc-backbone.js
Outdated
| /* global Backbone */ | ||
| if(!_.isUndefined(Backbone)) { | ||
| OC.Backbone = Backbone.noConflict(); | ||
| if(!_.isUndefined(Backbone)) { |
There was a problem hiding this comment.
if it's undefined the line above will fail hard 🙈
There was a problem hiding this comment.
in fact it's impossible that the var is undefined here. L12 prevents undefined values and then the object reference isn't changed. So it's always defined. Did you mean to check window.Backbone?
There was a problem hiding this comment.
Backbone.noConflict() can set Backbone back to undefined. Use your debugger 🙈
There was a problem hiding this comment.
js scopes are fun, aren't they? :-) The local parameter/variable Backbone hides the global variable you were referring to I guess
rullzer
force-pushed
the
only_init_backbone_once
branch
from
8f09dab to
2069867
Compare
core/js/oc-backbone.js
Outdated
| /* global Backbone */ | ||
| if(!_.isUndefined(Backbone)) { | ||
| OC.Backbone = Backbone.noConflict(); | ||
| if(_.isUndefined(window.Backbone)) { |
There was a problem hiding this comment.
window.Backbone is basically the same as Backbone 😉
There was a problem hiding this comment.
yeah. pfff I hate this... might be better not to do magic and just fix the apps.
|
nextcloud/external#9 is the fix for externals to work again (without this nasty fix). |
| OC_Util::addScript('select2-toggleselect'); | ||
|
|
||
| OC_Util::addScript('oc-backbone', null, true); | ||
| OC_Util::addVendorScript('core', 'backbone/backbone', true); |
There was a problem hiding this comment.
I would just remove this in this PR and then it's fine.
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
rullzer
force-pushed
the
only_init_backbone_once
branch
from
2069867 to
e4d4fb5
Compare
|
@nickvergessen I'd like you to give it a test spin as well |
We loaded backbone.js twice. Which caused the initialization to run twice.
Because of https://github.com/nextcloud/server/blob/master/core/js/oc-backbone.js#L13 when only loading backbone once the global
Backbonewill be set to undefined.@nickvergessen this was the issue you had with #3795
Best solution is to always use OC.Backbone instead of Backbone directly. But this fix makes it work with Backbone again as well.