fix($compile): validate directive name for whitespaces #11772
Conversation
|
I think you would also need a new |
|
LGTM, provided that an existing error page is used and its content is updated accordingly |
|
@pkozlowski-opensource Is the updated error page good enough for you, or probably you would like to structure conditions in a list? |
|
Should be fine as a starting point. Could you please squash all the 3 commits into one so it is easier to merge? |
Throw an exception if directive name contains leading or trailing whitespaces Closes angular#11397
lugovsky
force-pushed
the
directive-name-spaces
branch
from
4395dc9 to
d92a237
Compare
|
@pkozlowski-opensource Sorry for interrupting you again. I can't understand why CI build failed. Don't see any errors related to my changes. Could you advice on that? :) |
|
All is well now :) |
|
@pkozlowski-opensource and @gkalpak can you assign this to a milestone and/or deal with it? |
|
It LGTM as is, but if we wanted to go the extra mile, we could verify there is no whitespace in the name whatsoever. |
|
Is this a breaking change or did the compiler just fail to match directives with leading/trailing whitespace before? Should we just be more compliant and simply trim directive names? |
|
Previously, the compiler just failed to match the directives, so this is not a breaking change. If we decide to also check for internal whitespace, then I guess throwing makes more sense. |
Personally I would only add additional checks if users keep complaining about common mistakes. Each check cost us frw size and CPU cycles, so checks should be used with moderation, IMO. |
|
Ah, and I'm for throwing - I really don't like systems that try to work-around common mistakes, Fail fast is the way to go IMO. |
|
OK, let's merge this then. |
…tespace Closes angular#11397 Closes angular#11772
Throw an exception if directive name contains leading or trailing whitespaces
Closes #11397