Make Region.setElement private

There is a possibility of attach issues when the `el`s are not in the same attached state, so for now this'll be internal use only
marionettejs · Apr 20, 2019 · 80f0018 · 80f0018
1 parent 30b10e6
commit 80f0018
Show file tree

Hide file tree

Showing 3 changed files with 20 additions and 15 deletions.
diff --git a/src/collection-view.js b/src/collection-view.js
@@ -74,7 +74,7 @@ const CollectionView = Backbone.View.extend({
     const $emptyEl = this.$container || this.$el;
 
     if (this._emptyRegion && !this._emptyRegion.isDestroyed()) {
-      this._emptyRegion.setElement($emptyEl[0]);
+      this._emptyRegion._setElement($emptyEl[0]);
       return this._emptyRegion;
     }
 

diff --git a/src/region.js b/src/region.js
@@ -30,7 +30,7 @@ const Region = function(options) {
   // Handle when this.el is passed in as a $ wrapped element.
   this.el = this.el instanceof Backbone.$ ? this.el[0] : this.el;
 
-  this._setElement();
+  this.$el = this._getEl(this.el);
 
   this.initialize.apply(this, arguments);
 };
@@ -95,16 +95,20 @@ _.extend(Region.prototype, CommonMixin, {
     return this;
   },
 
-  _setElement() {
-    if (!this.el) {
+  _getEl(el) {
+    if (!el) {
       throw new MarionetteError({
         name: classErrorName,
         message: 'An "el" must be specified for a region.',
         url: 'marionette.region.html#additional-options'
       });
     }
 
-    this.$el = this.getEl(this.el);
+    return this.getEl(el);
+  },
+
+  _setEl() {
+    this.$el = this._getEl(this.el);
 
     if (this.$el.length) {
       this.el = this.$el[0];
@@ -117,7 +121,7 @@ _.extend(Region.prototype, CommonMixin, {
   },
 
   // Set the `el` of the region and move any current view to the new `el`.
-  setElement(el) {
+  _setElement(el) {
     if (el === this.el) { return this; }
 
     const shouldReplace = this._isReplaced;
@@ -126,7 +130,7 @@ _.extend(Region.prototype, CommonMixin, {
 
     this.el = el;
 
-    this._setElement();
+    this._setEl();
 
     if (this.currentView) {
       const view = this.currentView;
@@ -194,7 +198,7 @@ _.extend(Region.prototype, CommonMixin, {
 
   _ensureElement(options = {}) {
     if (!_.isObject(this.el)) {
-      this._setElement();
+      this._setEl();
     }
 
     if (!this.$el || this.$el.length === 0) {

diff --git a/test/unit/region.spec.js b/test/unit/region.spec.js
@@ -139,6 +139,7 @@ describe('region', function() {
     });
   });
 
+  // NOTE: Currently an internal API with potential for public release
   describe('when setting the region element', function() {
     let TestView;
     let region;
@@ -155,29 +156,29 @@ describe('region', function() {
     });
 
     it('should return the region', function() {
-      expect(region.setElement(twoEl)).to.equal(region);
+      expect(region._setElement(twoEl)).to.equal(region);
     });
 
     it('should set the el', function() {
       region.show(new TestView());
-      region.setElement(twoEl);
+      region._setElement(twoEl);
       expect(region.el).to.equal(twoEl);
     });
 
     it('should set the $el', function() {
       region.show(new TestView());
-      region.setElement(twoEl);
+      region._setElement(twoEl);
       expect(region.$el[0]).to.equal($(twoEl)[0]);
     });
 
     it('should throw an error if the `el` is not specified', function() {
-      expect(region.setElement.bind(region)).to.throw();
+      expect(region._setElement.bind(region)).to.throw();
     });
 
     describe('when setting the `el` to the same element', function() {
       it('should not requery the el', function() {
         this.sinon.spy(region, 'getEl');
-        expect(region.setElement(oneEl)).to.equal(region);
+        expect(region._setElement(oneEl)).to.equal(region);
         expect(region.getEl).to.not.be.called;
       });
     });
@@ -186,7 +187,7 @@ describe('region', function() {
       it('should replace the el of the region with the view el', function() {
         const view = new TestView();
         region.show(view, { replaceElement: true });
-        region.setElement(twoEl);
+        region._setElement(twoEl);
         expect($('#region1')).to.be.lengthOf(1);
         expect($('#view')).to.be.lengthOf(1);
         expect($('#region2')).to.be.lengthOf(0);
@@ -197,7 +198,7 @@ describe('region', function() {
       it('should attach the view html to the region', function() {
         const view = new TestView();
         region.show(view, { replaceElement: false });
-        region.setElement(twoEl);
+        region._setElement(twoEl);
         expect($('#region1')).to.be.lengthOf(1);
         expect($('#region1 #view')).to.be.lengthOf(0);
         expect($('#region2 #view')).to.be.lengthOf(1);