fix(animations): handle structured AnimateTimings

[chrisguttandin]

Fixes: #22752

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

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: 22752

What is the new behavior?

Due to the missing return statement structured AnimateTimings where tried to parse as if they were a string. Now the function returns early if there is no need to parse the input.

Does this PR introduce a breaking change?

• Yes
• No

Other information

I would be happy to add an additional test for this but I wasn't able to find the corresponding test suite.

[jessicajaniuk]

@crisbeto Could you review this PR? It looks pretty small, but you're the most familiar with the animations package.

[chrisguttandin]

It's been a while since I created this PR. :-) Please let me know if anything needs to be changed.

[petebacondarwin]

One addition that would be welcome is if you could put together a unit test that demonstrated the bug and passes after this fix.

[petebacondarwin]

I wonder if it would be better to just add the code from line 542-549 into an else block, so that it doesn't get run if the value was an object or a number?

[crisbeto]

Also as Pete mentioned, we should have a unit test for this.

[crisbeto]

I would change the function to something like the following so it's easier to follow:

function constructTimingAst(value: string|number|AnimateTimings, errors: any[]) {
  if (value.hasOwnProperty('duration')) {
    return value as AnimateTimings;
  }
  
  if (typeof value == 'number') {
    const duration = resolveTiming(value, errors).duration;
    return makeTimingAst(duration, 0, '');
  }

  const strValue = value as string;
  const isDynamic = strValue.split(/\s+/).some(v => v.charAt(0) == '{' && v.charAt(1) == '{');
  if (isDynamic) {
    const ast = makeTimingAst(0, 0, '') as any;
    ast.dynamic = true;
    ast.strValue = strValue;
    return ast as DynamicTimingAst;
  }

  const timings = resolveTiming(strValue, errors);
  return makeTimingAst(timings.duration, timings.delay, timings.easing);
}

[chrisguttandin]

If I recall correctly I thought it would be better to fix the problem with the least amount of code possible when I initially created this PR. But I agree that your solution is much cleaner. I updated the PR accordingly.

[jessicajaniuk]

@chrisguttandin If you have a chance to respond to the comments here, that'd be great. We'd love to land this. Thanks!

[chrisguttandin]

@jessicajaniuk Sorry for the delay. I rebased the PR and applied the feedback but I did not yet add a test. I'm sure I'm missing something obvious but I can't find a guide on how to run the tests for the animations package locally. Is there a way to do this?

[jessicajaniuk]

@chrisguttandin It depends on where the test is exactly. You can try yarn bazel test packages/animations/browser/test and add --config=debug if you want to run them locally.

[chrisguttandin]

Thanks a lot @jessicajaniuk. I added a test case which resembles the example from the original issue #22752. I also rebased the branch again and resolved the conflicts.

Please let me know if there is anything else that needs to be changed.

[jessicajaniuk]

LGTM 🍪

Sorry for this taking so long to get approved!

[jessicajaniuk]

@chrisguttandin Looks like there's been some linting changes since this PR last had CI run. If you can run the command in the lint CI failures to reformat the files, that would be wonderful. It's otherwise green ad can be landed as soon as you can push that.

[AndrewKushnir]

This PR was merged into the repository by commit a3f344f.

[angular-automatic-lock-bot]

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.}