refactor($compile): remove unnecessary try/catch

[jbedard]

I don't see anything in there that could throw...

[gkalpak]

It could throw if directive.restrict does not have an .indexOf method (e.g. it is not a String; nor an array). Funny enough (and although unintented afaict), using an array works 😃

See both in action here.

Since I don't think we want to keep a non-string, user-specified directive.restrict, I suggest keeping this change, but also changing the line that sets directive.restrict from:

directive.restrict = directive.restrict || 'EA';

...to:

directive.restrict = isString(directive.restrict) && directive.restrict || 'EA';
// or
directive.restrict = (directive.restrict && isString(directive.restrict)) ? directive.restrict : 'EA';

[jbedard]

There are plenty of other directive properties without validation (priority, link, name, require, ...). But I'd be fine doing it if you'd like, better once in validation then a try/catch at runtime...

[gkalpak]

I don't feel strongly either way (as they are both better than a try-catch).

You said you didn't see anything that could throw - just pointing out there is 😛

[jbedard]

True enough 😄

[Narretz]

@gkalpak With your proposed change, you wouldn't notice that you have a wrong restrict setting, right? Because it would silently use 'EA' in that case.

[gkalpak]

@Narretz, yes, but that is already the case if you have a falsy value or a wrong value (e.g. 'XYZ'). In both cases your directive won't work as expected, but there won't be an error.

Do you think it's better to log a warning ?
The idea is to be able to safely remove the try-catch, without crashing the whole app if someone mistyped their restrict.

[Narretz]

@gkalpak I see what you mean now. I guess we could log a debug / warning note. It would be pretty unusual for us, though.

[jbedard]

I think logging such a thing would be unusual and inconsistent with all the other DDO properties that could be invalid (and are not checked)...

[Narretz]

I'm still a little uneasy with merging this, as we remove the possibility to handle the exception. I don't think that anyone uses this, but you never know. And in that respect it would be a BC.

[jbedard]

I still don't see any issue with that, we don't do such validation on any other property. But if we did want validation I'd think it should be done on directive instantiation instead of each and every attempt at using the directive.

[jbedard]

Any thoughts on this? Allow it to throw in addDirective (like this PR currently changes it to do)? If we want to do validation of directive properties I think it should be in registerDirective...

[gkalpak]

I agree with @Narretz that this is a (tiny, highly unlikely to affect anyone) BC.
The problem is that currently, if a directive has a bad restrict value, it will fail, but other directives will work fine. With this change, the problem of one directive, breaks out of the loop and might affect other directives.

So, an app that is working without this change (a simply logs an error for one broken directive), could stop working with this change.

So, I would not merge it into 1.5.
If it offers a non-negligible perf improvement, I would get it into 1.6 (with a BC notice).

_{("non-negligible" is subjective, so I leave room for interpretation 😛 )}

[jbedard]

If we want to do such validation how about moving it to directive registration instead of on each and every compilation of that directive? Then the user will get one error logged when the directive factory runs instead of 1 per compilation of the directive.

[gkalpak]

Just to be clear, I don't feel strongly about keeping the try-catch block or doing any validation. I just want to avoid breaking a working app.

So, to that end, since directive.restrict.indexOf is the only part that could (reasonably) throw here, I'm fine replacing the try-catch with a check that directive.restrict.indexOf exists/is a function (either inside addDirectives or during registration if that doesn't break the current behavior of decorated directives).

It's debatable if we want to just skip directives with an invalid restrict property or call the $exceptionHandler().

[jbedard]

I've added an additional commit that validates the restrict property when the directive factory runs. This way errors are caught and logged but only once per directive instead of once per compilation of the directive like previously. The validation could be a bit stricter, like not allowing "foobar!@-E ", but that would actually be a breaking change so I didn't go that far (the "E" restricts it to elements).

[gkalpak]

I would say "...string including one of more of the following characters:".
It might be also worth adding a couple of example: restrict: 'E', restrict: 'EAC'

[gkalpak]

You can keep the rethrow mode and use the toThrowMinErr helper instead.

[jbedard]

By default in tests $exceptionHandler seems to throw. I had to change that to only log so that we can test that it only invokes $exceptionHandler once (on first usage of the directive) and any further use of the directive does not re-log the same error message.

[jbedard]

I've updated this to convert all falsey values to the default like previously.

Cases which previously logged errors per-compilation (of an attribute/element with the same name), but otherwise did nothing:

• non-string truthy values

Cases which previously silently did nothing:

• arrays without single-character E/A/C/M
• strings with only invalid characters

Cases that previously worked unintentionally:

• arrays with single-character entries of E/A/C/M (such as ['E', 'A'] but not ['EA'])

Now all of these cases get logged as errors the first time an attribute/element of that name gets compiled.

[jbedard]

I think I've addressed the comments. Let me know if there's anything else, or if you can prove I'm wrong again :)

[gkalpak]

Looking better 😃 We have two possible BCs now:

1. Directives using an array - either accidentally working (e.g. ['E', 'A']) or not working (e.g. ['EA']).
2. Directives using string containing non-EACM characters only (e.g. 'X').

I'm OK with the first category, since it's quite far-fetched and only worked accidentally. It's probably a programming error and throwing is a good thing.

What bother's me a little with the second BC, is that (in far-fetched usecase) one could use the restrict property to determine whether on not the directive is active, based on a condition checked at runtime. E.g.:

.directive('superAdminActions', function (User) {
  return {
    restrict: User.isSuperAdmin() ? 'E' : 'X',
    ...
  };
})

It's a little contrieved, but not impossible imo 😃 So, while I don't think we need to support this, I would still land this change in 1.6.x only (with a proper BC notice).

Other than that, LGTM 😃

[jbedard]

For 1), do we care? Should we support the array case? I would vote no...

For 2) note that this change only logs an error and prevents that directive from being added to the list of directives. The logged error will be new, but it shouldn't actually break anything.

[gkalpak]

For 1), do we care? Should we support the array case? I would vote no...

Me too

For 2) note that this change only logs an error and prevents that directive from being added to the list of directives. The logged error will be new, but it shouldn't actually break anything.

The logged error already is a BC imo. But here is a super contrived example of a usecase that would be totally broken: you broke me 😛

[jbedard]

Is logging an error really a BC? It just adds information to inform the user of an error that they otherwise might not notice. Unless they purposely crash the app via $exceptionHandler I guess...

However your example does show a potential BC, but honestly that seems like something that shouldn't even be supported: changing directive properties after a directive has already been registered. Your example was changing restrict, what about changing link or controller/name, those would already have issues today (I think). I'd say we should go the opposite direction and do a copy(directive) to remove support for modifying any directive properties once the directive factory as run...

[gkalpak]

I'd say we should go the opposite direction and do a copy(directive)

Now that would be a big BC.

I'm not too bothered by any of the two BCs. If others think it is not a BC (i.e. it was never intended to work like this and if you are relying on that specific behavior it's your bad), I'm fine shipping it.
_{(But if people start complaining and we revert it, I will say "I told you" 😛 )}

[jbedard]

Now that would be a big BC.

Do you really think people abuse this today? I think preventing modifications to the DD would make things more consistent, and allow angular to do things like parsing or validation in the factory function (like this PR) instead of per-compile/link.

[gkalpak]

Now that would be a big BC.

Do you really think people abuse this today?

I do :) I think there are people that try to access stuff off the DDO (e.g. using this inside some of the functions - template/templateUrl/compile/link). Some might define a ddo object and refer to that explicitly, e.g.:

var ddo = {
  ...
  compile: function () {
    ddo.xyz...
  }
};

return ddo;

I'm not saying these are common usecases, but we shouldn't break uncommon usecases either between minor versions. I'm fine with the change if you think it has benefits, but for 1.6.x only (not 1.5.x).

[jbedard]

Is 1.6 stuff being merged yet?

I do think isolating try/catch like this is worth it, similar to the recent acd4551 and many previous changes that did the same...

[Narretz]

Current master === 1.6
So there's no BC here anymore? If so I'll merge it once you have squashed the commits. (Please include the comment on the conversion / check in the commit message)

[jbedard]

I've squashed and updated the commit message, and fixed a new eslint ' vs " error.

As for breaking changes, only weird edge cases which were never intentionally supported will throw the error. Let me know if this is not clear from the commit message.