feat(DirectiveParser): throw errors when expected directives are not... by b-laporte · Pull Request #570 · angular/angular

b-laporte · 2015-02-06T23:47:40Z

... present (i.e. when non-root templates have no directive or when invalid DOM properties that are associated to an expression are not matched by any directive...)

fix #527

vicb · 2015-02-08T10:19:44Z

StringMapWrapper would be faster (ES5) if keys are string

Indeed - I will do the change asap.

mhevery · 2015-02-10T04:21:06Z

Could the error message include the Element HTML so that developer would know where to go to fix it?

I updated the PR with more explicit context information. For the time being it is a simple HTML description - but if we move to a JS parser (cf. webworkers) the generated DOM could include meta-data for each element such as file name, line and column numbers that could be then used for even more meaningful messages.

mhevery · 2015-02-10T04:21:26Z

Looks good to me, but I would like @tbosch to take another look.

b-laporte · 2015-02-16T17:26:07Z

I did a few refactoring since the first review:

tests have been moved to integration tests to not create false positives while mocking the first compilation steps (that are also involved in error handling btw.)
a helper has been added on NgElement to generate the element description that is included in each error message (maybe there could be a better place for this helper?)
it seems like the Promise.catch is not properly transpiled to Future.catchError in DART - this is why some tests are skipped in dart in this PR (cf. isObject({}) test) as I didn't find any way to have tests running on both environments (if I use catch() instead of then() the dart code will not even load - this is why I used the Promise.then form in the tests)

I also realised that the DOM.attributeMap() method was relying on a Map and not a StringMap - any reason for this? I guess this could be changed for better performances?

@tbosch I think I am done with this PR - so feel free to comment - thx 😃

tbosch · 2015-02-18T17:15:24Z

We should fill this already here, not and lazily by a compile step, as previous compile steps might have changed the element already. Also, this should include the compilationUnit. Right now we pass the compilationUnit as constructor argument into some steps (e.g. PropertyBindingParser), so we can provide a more meaningful error when parsing expressions. But we should actually also use the elementDescription + compilationUnit there as well.

I.e. remove the compilationUnit from the constructors of the various compile steps and include it in the elementDescription, then use the new elementDescription where the compilationUnit was used before.

Only if assertionsEnabled() though...

tbosch · 2015-02-18T17:44:02Z

Don't use a generic fail method, but provide an error description at the place where you want to fail (e.g. should not have come here as an exception was expected, ...)

tbosch · 2015-02-18T17:52:23Z

LGTM, besides the mentioned changes. Also needs rebase ontop of master...

b-laporte · 2015-02-19T08:58:02Z

thanks for the comments - I will provide an update in the coming days (unfortunately don't have much time right now..)

tbosch · 2015-02-19T16:34:28Z

Thanks!
On Thu, Feb 19, 2015 at 12:58 AM Bertrand Laporte notifications@github.com
wrote:

thanks for the comments - I will provide an update in the coming days
(unfortunately don't have much time right now..)

—
Reply to this email directly or view it on GitHub
#570 (comment).

b-laporte · 2015-02-22T18:21:13Z

OK - cleanup done according to @tbosch comments. Some more comments regarding this PR:

the compile element element description is now calculated in compile_element but is still overridden in view_splitter for template elements that are created dynamically so that users get a meaningful error messages (otherwise messages will mention elements that don't appear in the original template file)
In element_binder_builder I cannot write const STYLE_ATTR = 'style';const STYLE_PREFIX = STYLE_ATTR + '.'; as I get the following error with DART: dart error: initializer is not a valid compile-time constant
I preferred to keep some of the test in integration_spec as the code handling the errors in spread in view_splitter, directive_parser, compile_element and element_binder_builder. It also reduces the risk to create false positives (i.e. errors than cannot happen in real life but that can be generated through incomplete mocks) - and the error validation will be easier to maintain while the code evolves as samples represent what users will write in real templates.
regarding isSpecialProperty() (new name of previous isValidProperty), I originally thought that DOM.hasProperty() was checking for valid properties - whereas I just realized it checks for attributes (so I guess name should be DOM.hasAttribute) - this is why in my original version I was checking role , class and style which are valid attributes, but not properties.

…present closes angular#527

…present closes #527 Closes #570

Closes #787

angular-automatic-lock-bot · 2019-09-06T01:21:26Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

naomiblack added the in progress label Feb 6, 2015

googlebot added the cla: yes label Feb 6, 2015

b-laporte force-pushed the directive_validation branch from ac64440 to f116c20 Compare February 7, 2015 00:38

vicb reviewed Feb 8, 2015
View reviewed changes

b-laporte force-pushed the directive_validation branch from dac45a1 to b7e0ed8 Compare February 9, 2015 17:30

mhevery reviewed Feb 10, 2015
View reviewed changes

mhevery assigned tbosch Feb 10, 2015

mhevery added @lgtm action: merge The PR is ready for merge by the caretaker labels Feb 11, 2015

mhevery modified the milestone: M5: Early Adopters Feb 11, 2015

b-laporte force-pushed the directive_validation branch 4 times, most recently from 85d56a8 to 2676e69 Compare February 15, 2015 21:59

mhevery force-pushed the master branch from 9e4510a to cf51057 Compare February 17, 2015 23:07

tbosch reviewed Feb 18, 2015
View reviewed changes

tbosch added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 18, 2015

b-laporte force-pushed the directive_validation branch from 2676e69 to 8995a73 Compare February 22, 2015 18:14

b-laporte force-pushed the directive_validation branch from 8995a73 to acdf898 Compare February 22, 2015 20:27

feat(DirectiveParser): throw errors when expected directives are not …

c87fd04

…present closes angular#527

b-laporte force-pushed the directive_validation branch from acdf898 to c87fd04 Compare February 22, 2015 20:58

b-laporte pushed a commit that referenced this pull request Feb 24, 2015

feat(DirectiveParser): throw errors when expected directives are not …

90437cf

…present closes #527 Closes #570

b-laporte closed this in 94e203b Feb 25, 2015

naomiblack removed the in progress label Feb 25, 2015

b-laporte mentioned this pull request Feb 25, 2015

fix rebase glitch in PR #570 #787

Closed

b-laporte pushed a commit to b-laporte/angular that referenced this pull request Feb 25, 2015

fix rebase glitch in PR angular#570

d3216fa

b-laporte pushed a commit that referenced this pull request Feb 25, 2015

fix rebase glitch in PR #570

33b5037

Closes #787

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 6, 2019

Conversation

b-laporte commented Feb 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhevery commented Feb 10, 2015

Uh oh!

b-laporte commented Feb 16, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbosch commented Feb 18, 2015

Uh oh!

b-laporte commented Feb 19, 2015

Uh oh!

tbosch commented Feb 19, 2015

Uh oh!

b-laporte commented Feb 22, 2015

Uh oh!

angular-automatic-lock-bot Bot commented Sep 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants