fix(ngOptions): don't throw if options are unset inside writeValue

[Narretz]

This can happen in the following case:

• there's a blank option inside the select
• another directive on the select element compiles the contents of it before ngOptions is linked.

Now this happens:

• the option directive is compiled and adds an element $destroy listener that calls ngModel.$render
• when ngOptions processes the blank option, it removes the element, and
  triggers the $destroy listener
• ngModel.$render delegates to selectCtrl.writeValue, which accesses the options
• in that phase, the options aren't yet set

Fixes #11685

[Narretz]

I think the check is reasonable, even if the underlying problem is a bit iffy. Basically, the option directive does its thing even if ngOptions is on the select parent. So we could also introduce a check inside the option directive to prevent this. However, this really just happens if somehow the contents of the select are compiled before ngOptions gets to it, because ngOptions removes the blank option before it compiles it (and it therefore doesn't have a select parent and does nothing at all).

[gkalpak]

Too much indentation ?

[petebacondarwin]

another directive on the select element compiles the contents of it before ngOptions is linked.

The problem here is that ngOptions is terminal: true, which means it makes the assumption that the option elements below it are not compiled ... ever. If we manually compile these options then they will get hold of the SelectController and add themselves. I suggest that a better fix for this issue is to get ngOptions to also override SelectController.addOption and SelectController.removeOption with noops so that even if these options are compiled the do not get registered with the SelectController at all.

What do you think? I am not sure, yet, whether this would fix the other part of this issue.

[Narretz]

If both directives have priority 0, they should be both executed (at least according to the docs). However, I just noticed that my current test actually passes without the change, because compileContent is not executed, but rompileContents is, because r comes after n. This sounds a bit fishy to be honest.

Overwriting addOption and removeOption is good for consistency, but it won't fix the bug, because the call in option that leads to the exception comes from element.$on('$destroy')

[Narretz]

Correction: rompileContents is linked before ngOptions, compileContents after it, so that's why it always works in the second case.

[petebacondarwin]

OK, so the best trick is to set the SelectController's ngModelCtrl property to null during the pre link function for ngOptions. This effectively "turns off" the select directive, from the point of view of the option directive and is really what ngOptions would like anyway.

[Narretz]

That doesn't work, because the order is:

• ngOptions pre
• options post
• option post

So the ngModelCtrl prop is set after ngOptions pre runs.

[petebacondarwin]

Take a look at this #12972

[petebacondarwin]

It is not quite right as you say but I feel that this is a good direction to be looking for a fix.

[petebacondarwin]

Actually I take it back. Setting $modelCtrl to null alone doesn't fix the problem...

[Narretz]

Let's get a bit crazy:

• give select priority: 1, so it's preLink fn is run before ngOptions
• move (almost) all select ctrl logic to preLink fn (is it problematic to move element eventlisteners there?)
• override selectCtrl.ngModelCtrl inside ngOptions's preLink fn.
• Tests pass
• ???
• Userland stuff breaks?

Honestly, the directive order stuff is so confusing, especially when you throw terminal in the mix.

[petebacondarwin]

The really tricky bit is when you have templateURL and replace because some of the directives run half of their code at weird times and arguably out of order.

[Narretz]

The commit contains a test for the replace directive + templateUrl.

[petebacondarwin]

I think I have the right fix - force pushed to #12972

[Narretz]

If the compileContents directive looks like below, it still breaks. This runs the preLinkl fn before select and ngOptions, so we're out of luck.

  .directive('oCompileContents', function() {
    return {
      priority: 2,
      link: {
        pre: function(scope, element)  {
          dump('oCompileContents pre')
          linkLog.push('linkCompileContents');
          $compile(element.contents())(scope);
        }
      }
    };
  });

with

    TypeError: Cannot read property '$render' of null
        at HTMLOptionElement.<anonymous> (c:/Users/Martin/static/angular.js/src/ng/directive/select.js:468:35)

[petebacondarwin]

Arguably that is an invalid use of Angular

[Narretz]

@petebacondarwin I updated the PR. Now it uses a function on the selectCtrl that sets a flag and additionally sets addOption and removeOption to noop.

[petebacondarwin]

Good stuff. Can we call the method disable ()?

[Narretz]

I didn't want to call it disable because disabled has meaning the html element context.

[petebacondarwin]

Oh, right. I see that could be confusing... (Although we are disabling the controller and not the element).
Trouble is I found that override didn't mean anything to me, and even knowing what we are doing, I had to do a double take. Mainly because I would expect an override method to be passing a parameter that would be used to "instead" of the original thing, do you know what I mean?
The only other word I can come up with is annul but that is totally esoteric and not intuitive either.
How about disableController or detachController?

[petebacondarwin]

I had another idea. I wonder if it is possible to actually remove the select controller from the DOM from inside the ngOptions directive...

[gkalpak]

I like detach better than disable. Or maybe suspend.

[Narretz]

@petebacondarwin I don't know if removing the selectCtrl completely is what we want. I've always understood that ngOptions replaces some functionality of the selectCtrl instead of replacing it completely. For example, there's this issue - #12915 The API is undocumented, but I can see how it makes sense to support hasOption for select with and without ngOptions.