Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(platform-browser): improve error message for missing animation tr… #41356

Closed
wants to merge 1 commit into from

Conversation

kirjs
Copy link
Contributor

@kirjs kirjs commented Mar 27, 2021

…igger

There are two reasons why this error can be called, but only one was covered before.

Fixes #15581

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #15581

What is the new behavior?

An example of a new error message is:

Found the synthetic property @myAnimation. Possible reasons:
  - "BrowserAnimationsModule" or "NoopAnimationsModule" are missing in your application.
  - There is no corresponding configuration for the animation named `@myAnimation` defined in the `animations` field of the `@Component` decorator (see https://angular.io/api/core/Component#animations).

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@kirjs
Copy link
Contributor Author

kirjs commented Mar 27, 2021

I'm not sure if the animation message is relevant for the server code.
Should I roll that one back?

@jessicajaniuk
Copy link
Contributor

I'm not sure if the animation message is relevant for the server code.
Should I roll that one back?

Leave it in. It's possible this error could happen server side depending on how the person configured things, and the NoopAnimationsModule would be what they should see there. So it's definitely helpful for debugging.

Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🍪

Thank you!

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kirjs thanks for improving the error message 👍

I've left a few comments, could you please have a look when you get a chance?

packages/core/test/animation/animation_integration_spec.ts Outdated Show resolved Hide resolved
packages/core/test/animation/animation_integration_spec.ts Outdated Show resolved Hide resolved
packages/platform-browser/src/dom/dom_renderer.ts Outdated Show resolved Hide resolved
packages/platform-browser/src/dom/dom_renderer.ts Outdated Show resolved Hide resolved
@AndrewKushnir AndrewKushnir added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews area: core Issues related to the framework runtime hotlist: error messages target: patch This PR is targeted for the next patch release labels Mar 29, 2021
@ngbot ngbot bot modified the milestone: Backlog Mar 29, 2021
@kirjs kirjs force-pushed the fix-15581-animation-message branch 3 times, most recently from 89be2b4 to acccedb Compare April 4, 2021 19:05
@kirjs
Copy link
Contributor Author

kirjs commented Apr 4, 2021

Addressed the feedback PTAL

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the feedback @kirjs 👍

I've left 1 minor comment, please have a look when you get a chance.

packages/platform-browser/src/dom/dom_renderer.ts Outdated Show resolved Hide resolved
@kirjs kirjs force-pushed the fix-15581-animation-message branch from 70716aa to 223be7e Compare April 11, 2021 19:33
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating this PR @kirjs. It looks there is a couple more places where we can replace double quotes with backticks (for consistency). Could you please have a look at the comments when you get a chance? Thank you.

packages/core/test/animation/animation_integration_spec.ts Outdated Show resolved Hide resolved
packages/core/test/animation/animation_integration_spec.ts Outdated Show resolved Hide resolved
packages/platform-server/src/server_renderer.ts Outdated Show resolved Hide resolved
@AndrewKushnir
Copy link
Contributor

@kirjs there are some lint warning as well, caused by the commit messages not being compliant with the format (the scope is missing, i.e. fix(...): part). Could you plz squash all commits into one once the feedback is addressed?

…igger

There are two reasons why this error can be called, but only one was covered before.

Fixes angular#15581
@petebacondarwin
Copy link
Member

@google I consent

@petebacondarwin
Copy link
Member

petebacondarwin commented Sep 15, 2021

I have rebased and fixed up the requested changes.

@petebacondarwin petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Sep 15, 2021
@petebacondarwin petebacondarwin removed the request for review from alan-agius4 September 15, 2021 14:32
Copy link
Contributor

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for platform-server

@petebacondarwin petebacondarwin added action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 15, 2021
@AndrewKushnir
Copy link
Contributor

Presubmit.

@AndrewKushnir
Copy link
Contributor

@petebacondarwin thanks for pushing updates! I've started a new presubmit and will add the PR to the merge queue once presubmit is completed. Thank you.

@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Sep 15, 2021
jessicajaniuk pushed a commit that referenced this pull request Sep 15, 2021
…igger (#41356)

There are two reasons why this error can be called, but only one was covered before.

Fixes #15581

PR Close #41356
@jessicajaniuk
Copy link
Contributor

Ignore the closed message. This PR has been merged through tooling.

TeriGlover pushed a commit to TeriGlover/angular that referenced this pull request Sep 16, 2021
…igger (angular#41356)

There are two reasons why this error can be called, but only one was covered before.

Fixes angular#15581

PR Close angular#41356
TeriGlover pushed a commit to TeriGlover/angular that referenced this pull request Sep 22, 2021
…igger (angular#41356)

There are two reasons why this error can be called, but only one was covered before.

Fixes angular#15581

PR Close angular#41356
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime cla: yes hotlist: error messages target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Misleading errormessage when using HostBinding with @animation trigger but no defined animations
5 participants