Skip to content

refactor(animations): deprecation of AnimationDriver.NOOP#51843

Closed
JeanMeche wants to merge 1 commit intoangular:mainfrom
JeanMeche:deprecate/static-noop-animation-driver
Closed

refactor(animations): deprecation of AnimationDriver.NOOP#51843
JeanMeche wants to merge 1 commit intoangular:mainfrom
JeanMeche:deprecate/static-noop-animation-driver

Conversation

@JeanMeche
Copy link
Copy Markdown
Member

@JeanMeche JeanMeche commented Sep 20, 2023 •
edited
Loading

The NoopAnimationDriver as static property of AnimationDriver prevents it from being removed by tree shaking. This commit deprecates it and exposes the NoopAnimationDriver on the public API to replace its usage.

DEPRECATED: AnimationDriver.NOOP

@JeanMeche JeanMeche marked this pull request as ready for review September 20, 2023 23:42
@pullapprove pullapprove bot requested a review from crisbeto September 20, 2023 23:42
alfaproject
alfaproject reviewed Sep 21, 2023
View reviewed changes
@JeanMeche JeanMeche force-pushed the deprecate/static-noop-animation-driver branch 2 times, most recently from ca828c9 to 2192f87 Compare September 21, 2023 08:01
@JeanMeche JeanMeche changed the title refactor(animations): deprecation of AnimationDriver.NOOP refactor(animations): deprecation of AnimationDriver.NOOP Sep 21, 2023
@JeanMeche JeanMeche force-pushed the deprecate/static-noop-animation-driver branch from 2192f87 to 1c8f7a4 Compare September 21, 2023 11:36
jessicajaniuk
jessicajaniuk approved these changes Sep 21, 2023
View reviewed changes
Copy link
Copy Markdown
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.

reviewed-for: public-api

goldens/public-api/animations/browser/index.md Outdated
Copy link
Copy Markdown
Contributor

@jessicajaniuk jessicajaniuk Sep 21, 2023

Choose a reason for hiding this comment

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

I know these all say undocumented, because they were never documented, but we should probably add docs for these.

Copy link
Copy Markdown
Member Author

@JeanMeche JeanMeche Sep 21, 2023

Choose a reason for hiding this comment

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

Let me know what you think of the doc I've added !

@pullapprove pullapprove bot requested a review from dylhunn September 21, 2023 15:33
AndrewKushnir
AndrewKushnir approved these changes Sep 21, 2023
View reviewed changes
Copy link
Copy Markdown
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.

LGTM, thanks for the refactoring 👍

Could you please update the commit message to include the deprecation note? Similar to what you have in PR description, but with some extra info, for example:

DEPRECATED:

The `AnimationDriver.NOOP` symbol is deprecated, use `NoopAnimationDriver` instead.

@pullapprove pullapprove bot requested a review from alxhub September 21, 2023 15:51
@AndrewKushnir AndrewKushnir added area: animations target: major This PR is targeted for the next major release labels Sep 21, 2023
@ngbot ngbot bot added this to the Backlog milestone Sep 21, 2023
@AndrewKushnir AndrewKushnir 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 Sep 21, 2023
@JeanMeche JeanMeche force-pushed the deprecate/static-noop-animation-driver branch from 1c8f7a4 to 50238c5 Compare September 21, 2023 16:42
@angular-robot angular-robot bot added the detected: deprecation PR contains a commit with a deprecation label Sep 21, 2023
@AndrewKushnir
Copy link
Copy Markdown
Contributor

AndrewKushnir commented Sep 21, 2023

@JeanMeche sorry I forgot to mention in the review comment: we'd also need to include a note on this into the deprecation guide (https://angular.io/guide/deprecations). Could you please add an item there (based on the deprecation message)?

@JeanMeche
refactor(animations): deprecation of AnimationDriver.NOOP
f0110aa 
The `NoopAnimationDriver` as static property of `AnimationDriver` prevents it from being removed by tree shaking. This commit deprecates it and exposes the `NoopAnimationDriver` on the public API to replace its usage.

DEPRECATED:
The `AnimationDriver.NOOP` symbol is deprecated, use `NoopAnimationDriver` instead.
@JeanMeche JeanMeche force-pushed the deprecate/static-noop-animation-driver branch from 50238c5 to f0110aa Compare September 21, 2023 21:40
@JeanMeche
Copy link
Copy Markdown
Member Author

JeanMeche commented Sep 21, 2023 •
edited
Loading

@AndrewKushnir Of course ! Deprecation.md has a new entry now !

@github-actions
Copy link
Copy Markdown

github-actions bot commented Sep 21, 2023

Deployed aio for f0110aa to: https://ng-dev-previews-fw--pr-angular-angular-51843-ma516312.web.app

Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt.

AndrewKushnir
AndrewKushnir approved these changes Sep 21, 2023
View reviewed changes
Copy link
Copy Markdown
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 👍

@AndrewKushnir AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit 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 21, 2023
AndrewKushnir
AndrewKushnir approved these changes Sep 22, 2023
View reviewed changes
Copy link
Copy Markdown
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.

Reviewed-for: public-api

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: presubmit The PR is in need of a google3 presubmit labels Sep 22, 2023
@AndrewKushnir
Copy link
Copy Markdown
Contributor

AndrewKushnir commented Sep 22, 2023

Caretaker note: windows CI failure is pre-existing, this PR is ready for merge.

@dylhunn
Copy link
Copy Markdown
Contributor

dylhunn commented Sep 22, 2023

This PR was merged into the repository by commit 0598613.

@dylhunn dylhunn closed this in 0598613 Sep 22, 2023
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot bot commented Oct 23, 2023

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 23, 2023
@JeanMeche JeanMeche deleted the deprecate/static-noop-animation-driver branch December 24, 2023 14:42
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
@JeanMeche @ChellappanRajan
refactor(animations): deprecation of AnimationDriver.NOOP (angular#…
b8a510e 
…51843)

The `NoopAnimationDriver` as static property of `AnimationDriver` prevents it from being removed by tree shaking. This commit deprecates it and exposes the `NoopAnimationDriver` on the public API to replace its usage.

DEPRECATED:
The `AnimationDriver.NOOP` symbol is deprecated, use `NoopAnimationDriver` instead.

PR Close angular#51843
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@crisbeto crisbeto crisbeto approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

+2 more reviewers

@alfaproject alfaproject alfaproject left review comments

@jessicajaniuk jessicajaniuk jessicajaniuk approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: animations detected: deprecation PR contains a commit with a deprecation merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

6 participants

@JeanMeche @AndrewKushnir @dylhunn @alfaproject @crisbeto @jessicajaniuk