Deprecate region @ui selectors

[paulfalgout]

Regions can currently use the @ui selector for their el selector.
This I believe is a rather expensive lookup that is only used for regions found here:
https://github.com/marionettejs/backbone.marionette/blob/master/src/mixins/ui.js#L25

This provides limits usefulness while requiring an expensive process. In the future I think regions may perhaps be able to be defined with a view's ui element directly if need-be, but we should remove the @ui. helper here.

[blikblum]

I would deprecate UI stuff as a whole or at least its usage in events / triggers keys. Its concept is tied to jQuery way of manipulating DOM, that's not as ubiquitous as used to be. In my apps, even the ones that i use jQuery to manipulate the DOM i dont use UI

[paulfalgout]

hmm I've used @ui. selectors in triggers almost exclusively to avoid multiple selector declarations. However with regions I don't find that they really even should overlap with a ui element. I'm open to possibly removing it at some point, but that's more aggressive and I think a second issue to removing it for regions only which to me has a higher complexity than it's worth.

[paulfalgout]

What I would support in the near future is putting the @ui. behind a feature flag.. even if the regions @ui. isn't deprecated

[paulovieira]

This is an interesting debate because I guess different people use @ui. in different ways.

In my case I make heavy use of it (including to mark the regions). I use it to distinguish beetween parts of the template that must not be touched when the template is changed, otherwise the view will not work as expected. I can look at it and know immediatelly if it's safe to change some part of the template.

So I fully agree with keeping the @ui. functionality for regions under some sort of flag. This also aligns nicely with other parts of marionette. For instance, I think the "attach" event can be turned off with some option if there's no need for it (because it trigger a long chain of events down the view tree).

[paulfalgout]

Thanks for the input. I think I agree. The unfortunate part of the region @ui is that it somewhat complicated and might be at unfortunate spots (as it has to search each region string per render regardless of if you use the feature)
I also think in the future that regions could utilize the ui directly instead of re-querying the DOM for the same selector.

[JSteunou]

I also use @ui a lot, and I'm more in favor of keeping it as a all everywhere, or remove it everywhere, for consistency.

Removing it only for region is not consistent and maybe to soon. Another move would be to upgrade the way regions are used / declare, and after the new alternative is accessible, shutting down the old way.

[paulfalgout]

It's worth noting that @ui was around for triggers and events for a lot longer than for regions.
But I see multiple use-cases for removing it from regions.
For one if replaceElement is used on the region, you've now broken the ui selector.
Two here's the code for adding @ui to events and triggers:

const normalizeUIKeys = function(hash, ui) {
  return _.reduce(hash, (memo, val, key) => {
    const normalizedKey = normalizeUIString(key, ui);
    memo[normalizedKey] = val;
    return memo;
  }, {});
};

And here's the code only used for regions:

const normalizeUIValues = function(hash, ui, properties) {
  _.each(hash, (val, key) => {
    if (_.isString(val)) {
      hash[key] = normalizeUIString(val, ui);
    } else if (_.isObject(val) && _.isArray(properties)) {
      _.extend(val, normalizeUIValues(_.pick(val, properties), ui));
      /* Value is an object, and we got an array of embedded property names to normalize. */
      _.each(properties, (property) => {
        const propertyVal = val[property];
        if (_.isString(propertyVal)) {
          val[property] = normalizeUIString(propertyVal, ui);
        }
      });
    }
  });
  return hash;
};

And all of that code in v3 currently runs against el and selector of a region for every region every time a view is rendered. It's costly.

I think other selectors such as [data-region-name] can be used to make them clearly not for anything else without needing all of the work.

I am also concerned that needing to support this will make changing how regions work for v5 more difficult, and I think we can provide better ways to let users reuse ui elements for a region if need be.. but I'm still wondering considering replaceElement if that's really a good idea..

[JSteunou]

That's a fair and heavy point.

So what the plan would be ?

1. deprecate region @ui
2. add feature flag to run today without region @ui as opt in feature
3. remove region @ui in next major

[paulfalgout]

After feedback from the community I think the feature flag is ideal, but I would divide triggers/events @ui from regions with two different flags. That way anyone can get the perf benefits immediately.

Then I am going out on a limb here, but I think v4 should possibly introduce a NextView and/or NextRegion It may not be necessary, but it did make CollectionView much easier to work on. I suspect there'd be some way to re-add the @ui. for regions.. but not sure.. plus we might find a better way to do it. But.. let's say we do something crazy and at some point replaceElement becomes the default.. then @ui. is really pretty problematic. So I think I'm still supporting it's removal at some point, but it wouldn't be until v5 or later

[blikblum]

And all of that code in v3 currently runs against el and selector of a region for every region every time a view is rendered. It's costly.

With #3537, only el will be handled. By changing normalizeUIValues properties argument from an array to a string it would allow to simplify the code a bit.

The only problem is that will change the signature of a public method although a not documented one. Also i don't see many use cases for it

[paulfalgout]

I think we should likely privatize normalizeUIValues anyhow. only normalizeMethods has a use case IMO. FWIW I think this function was also at one time exposed as Marionette.normalizeUIValues so it is less public now that it has been. If this check was far less costly I wouldn't care as much..

[JSteunou]

I m using it to normalize @ui in stickit bindings and I m not the only one 😉

[blikblum]

I m using it to normalize @ui in stickit bindings and I m not the only one

What method do you use?

normalizeUIValues or normalizeUIKeys ?

Can you post a snippet of the code?

[paulfalgout]

Related #3174 @bazineta

[bazineta]

Here's our usage for stickit(). We probably have 800 source files dependent on this.

#-----------------------------------------------------------------------------#
# Imports
#-----------------------------------------------------------------------------#

import _          from 'lodash'
import Marionette from 'backbone.marionette'

#-----------------------------------------------------------------------------#
# Turn off the childViewEventPrefix feature.
#-----------------------------------------------------------------------------#

Marionette.setEnabled 'childViewEventPrefix', false

#-----------------------------------------------------------------------------#
# Save original Backbone.Stickit functions and the destroy function.
#-----------------------------------------------------------------------------#

stickit    = Marionette.View::stickit
addBinding = Marionette.View::addBinding
unstickit  = Marionette.View::unstickit
destroy    = Marionette.View::destroy

#-----------------------------------------------------------------------------#
# Stickit selectors can be hashes, strings, or undefined. We need to
# normalize each type.
#-----------------------------------------------------------------------------#

normalizeSelector = (selector, view) ->
  return switch
    when _.isObject selector then view.normalizeUIKeys selector
    when _.isString selector then view.normalizeUIString selector
    else selector

#-----------------------------------------------------------------------------#
# Shim the three standard Stickit calls into Marionette View objects so
# that we can use the @ui syntax for bindings; this eliminates repetition of
# possibly long and complex selectors and allows us to have one definition
# for them, in the ui hash or function for the View. We also need to patch
# the destroy function, as the remove method that's patched by Stickit is
# never actually called by Marionette.
#-----------------------------------------------------------------------------#

_.extend Marionette.View::,

  stickit: (optionalModel, optionalBindings) ->
    return stickit.call @,
      optionalModel,
      @normalizeUIKeys(optionalBindings or _.result(@, 'bindings') or {})

  addBinding: (optionalModel, selector, configuration) ->
    return addBinding.call @,
      optionalModel,
      normalizeSelector(selector, @),
      configuration

  unstickit: (optionalModel, optionalSelector) ->
    return unstickit.call @,
      optionalModel,
      normalizeSelector(optionalSelector, @)

  destroy: ->
    @unstickit() unless _.isEmpty @_modelBindings
    return destroy.apply @, arguments

#-----------------------------------------------------------------------------#
# Exports
#-----------------------------------------------------------------------------#

export default Marionette

#-----------------------------------------------------------------------------#

[paulfalgout]

Yep notably missing normalizeUIValues

[JSteunou]

You're right, mostly using normalizeUIKeys and normalizeUIString

[paulfalgout]

With upcoming changes to region I believe @ui. selectors can be handled very elegantly for Regions, so removing this will not be necessary.