WIP Next Region #3664

paulfalgout · 2019-04-28T19:13:25Z

An iteration and replacement for #3620

This is starting to feel really good.
This is a WIP and I'll add more notes here shortly, but I wanted to get it out for 👀
I do think there's a handful of things we can introduce in a v4.x as well as some small deprecations that can be made for the breaking changes.
There are actually few breaking changes I believe the breaking changes that are there are minor and like inconsequential for most users.
I haven't done perf tests yet, but I feel moderately confident this will be more performant, and again I haven't tested it, but I believe this makes rendering views non-destructive to children.

I will work on documenting changes as well as breaking up this PR into sane commits as well as making v4.x PRs for features and deprecations we can do now.

But this I think is a good start. @marionettejs/marionette-core

Breaking Changes

Region no longer accepts a jquery instances as el el: $el[0] should be used instead of el: $el This removes region's dependencies on jquery.
As previously discussed, the not documented allowMissingEl is removed, however the reasoning for adding this flag in the first place is not an issue with this revision
Both add:region and remove:region events were removed. They're even less useful now than they were previously, and there's a new hook for when regions are bound to the view bind:regions that should cover any use case this previously had. This should make rendering cheaper overall as well.
Regions no longer empty on render. This may be the biggest breaking change to watch out for. However for the general case where the user is setting the children in onRender the 2nd show will handle the empty effectively in the same way as before. But there may be some cases where conditional child renders will no longer empty out the previous view. Users will need to handle these cases now, but this also should allow children to be created once in an onReady and child views may not need to be re-rendered at all when the parent is rendered.
Not documented, but Region's @ui allowed the user to build complex selectors such as:
regionName: '@ui.foo @ui.bar' producing regionName: '#foo .bar'. This change prevents that and requires a single ui only. Not only is the complex selector not recommended anyways, this prevents needing to requery the selector and the already queried ui is used instead.
Not documented, but defining a Region on a view's region hash allowed the user to pass either and instance of a region or a region class.
```
 regions : {
   foo: myRegion
   bar: MyRegionClass
 }
```
Both of these options are no longer supported as an el selector is required to be managed by the view, and both of these options prevent that. However allowing the view to maintain regions outside of the view is problematic and certainly makes showChildView difficult to understand. While I doubt many people are using this feature anyways, the way to support these options is:
```
const MyRegionClass = Region.extend({ el: '#foo' }); // some predefined selector
const MyView = View.extend({
  initialize({ myRegion }) {
    this.myRegion = myRegion;
  },
  regions: {
    bar: {
      el: '#foo',
      regionClass: MyRegionClass
    }
  }
```
Though largely considered a bug View's region using el from child view, not own view #3320 if a user was setting up regions to be within a child view, this will no longer work. It should be documented however that using addRegion or addRegions after the view is rendered will query all of the children until the render in which it will only query the rendered template. It might be best to suggest simply adding a definition to myView.regions and then re-rendering.
Regions are no longer instantiated prior to View#initialize. This shouldn't really be an issue as getting a region will render and instantiation the region needed... but if someone was doing something strange and expecting the region init to be prior to the view's init, it would technically be breaking.
The el on a region prior to showing a view was possibly the selector (if instantiated with a selector) Now it will always be a DOM element or undefined if nothing was found by the query. It might be an issue for users who instantiate a region that does a global look up for DOM that didn't exist at the time of instantiation.
Region#reset was removed. In this context it is no longer needed.

New Features
View ready event indicating the best spot to attach children
Application region can now be defined as a function returning a region or region definition
Region#setElement was made public and properly handles the various attached states of the elements

coveralls · 2019-04-28T19:16:13Z

Coverage decreased (-0.5%) to 99.516% when pulling 1e40870 on paulfalgout:region-next into 5090557 on marionettejs:master.

marionettejs#3320 This is one extra loop, but the dom query could be much cheaper since no children will be attached, and it prevents unintended querying of the same selector in a child

Keeps support for edge case in `setElement` where the view was ready on `initialize`, but would be after `setElement`

paulfalgout · 2019-04-29T10:53:42Z

src/common/normalize-region.js

+  }
+
+  if (!_.isString(definition.el)) {
+    console.log(definition);


need to remove this

paulfalgout · 2019-04-29T10:54:09Z

test/unit/view.dynamic-regions.spec.js

-        onAddRegion: function() {},
-        onBeforeAddRegion: function() {}
-      });
+      this.MyView = Marionette.View.extend();


all of the test changes in this file relate to removing add:region / remove:region

paulfalgout · 2019-04-29T10:54:25Z