Skip to content

refactor(animations): change errors type from any to string #44726

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.

Sign up for GitHub

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

Jump to bottom
Closed
dario-piotrowicz wants to merge 1 commit into from
Closed

refactor(animations): change errors type from any to string #44726

dario-piotrowicz wants to merge 1 commit into from

Conversation

@dario-piotrowicz
Copy link
Contributor

@dario-piotrowicz dario-piotrowicz commented Jan 15, 2022 •
edited
Loading

errors in the animations code are of type any but are consistently
used as if there were strings, change any to string to make
typing more accurate

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:

Issue

Issue Number: N/A

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

I am not completely sure why they were anys in the first place, I'd imagine it was done in order to make the code more flexible 🤔

Anyways as far I can tell all errors are just strings in the animations code and there are also parts in which getting something different from a string would likely create undesirable results, mainly when combining them together for the generation of an error message, for example like so:

angular/packages/animations/browser/src/render/timeline_animation_engine.ts

Line 37 in a92a89b

`Unable to build the animation due to the following errors: ${errors.join('\n')}`);

here if errors could be more complex data structures such as objects, when converted to strings by join (unless they implement their own toString) would just produce "[object Object]" which of course would produce a pretty bad error message

Also I don't think this is part of any public api so it should be a safe change to make 🙂

@pullapprove pullapprove bot requested a review from alxhub January 15, 2022 21:23
@dario-piotrowicz
refactor(animations): change errors type from any to string
9ce81b9 
errors in the animations code are of type `any` but are consistently
used as if there were `string`s, change `any` to `string` to make
typing more accurate
@dario-piotrowicz dario-piotrowicz mentioned this pull request Jan 16, 2022
Closed
14 tasks
@alfaproject
Copy link
Contributor

alfaproject commented Jan 16, 2022

Weird that the errors being thrown are strings. That's a really bad practise.

By the way, your description is wrong. This is perfectly valid:

[new Error('one'), new Error('two')].join(', ');
// prints: `'Error: one, Error: two'`

@dario-piotrowicz
Copy link
Contributor Author

dario-piotrowicz commented Jan 16, 2022 •
edited
Loading

Weird that the errors being thrown are strings. That's a really bad practise.

They are not being thrown as strings, the errors there are practically a collection of error messages that then when it's the right time are grouped together and thrown as an Error so this actually seems to make enough sense to me (and this is done because during the animation dsl parsing when we actually don't want to throw right away but want to conclude and show the errors all together at the end of the process)

By the way, your description is wrong. This is perfectly valid:

[new Error('one'), new Error('two')].join(', ');
// prints: `'Error: one, Error: two'`

Yeah that's a good point, but yeah they are implementing their own toString as I mentioned in the description there, so I think that counts! 😜👍 (like also many other built-in objects would present nicer outputs too)

By the way, even if the errors as you mentioned would be shown in a readable way, I believe that it would still look pretty awkward since it would be like:
Unable to build the animation due to the following errors: Error: one, Error: two
instead of:
Unable to build the animation due to the following errors: one, two

AndrewKushnir
AndrewKushnir approved these changes Jan 18, 2022
View reviewed changes
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.

@dario-piotrowicz thanks for the cleanup 👍

@AndrewKushnir AndrewKushnir removed the request for review from alxhub January 18, 2022 05:39
@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Jan 18, 2022
@pullapprove pullapprove bot requested a review from alxhub January 18, 2022 05:39
@AndrewKushnir AndrewKushnir added area: animations target: patch This PR is targeted for the next patch release labels Jan 18, 2022
@ngbot ngbot bot modified the milestone: Backlog Jan 18, 2022
@AndrewKushnir AndrewKushnir removed the request for review from alxhub January 18, 2022 05:40
@pullapprove pullapprove bot requested a review from alxhub January 18, 2022 05:40
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Jan 18, 2022

Presubmit.

@AndrewKushnir AndrewKushnir requested review from jessicajaniuk and removed request for alxhub January 18, 2022 05:40
jessicajaniuk
jessicajaniuk approved these changes Jan 18, 2022
View reviewed changes
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 🍪

Thanks for cleaning this up!

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker refactoring Issue that involves refactoring or code-cleanup and removed action: presubmit The PR is in need of a google3 presubmit labels Jan 18, 2022
@dylhunn
Copy link
Contributor

dylhunn commented Jan 18, 2022

This PR was merged into the repository by commit ece530f.

@dylhunn dylhunn closed this in ece530f Jan 18, 2022
dylhunn pushed a commit that referenced this pull request Jan 18, 2022
@dario-piotrowicz @dylhunn
refactor(animations): change errors type from any to string (#44726)
929788d 
errors in the animations code are of type `any` but are consistently
used as if there were `string`s, change `any` to `string` to make
typing more accurate

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

angular-automatic-lock-bot bot commented Feb 18, 2022

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 Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

+1 more reviewer

@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 refactoring Issue that involves refactoring or code-cleanup target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

5 participants

@dario-piotrowicz @alfaproject @AndrewKushnir @dylhunn @jessicajaniuk