qonto / ember-lottie

An Ember.js component to render lottie after effects animations.
MIT License
17 stars 5 forks source link

Difference in implementation of the Lottie component compared to testing #20

Closed clairekrucker closed 1 year ago

clairekrucker commented 1 year ago

I have a problem with the implementation of the Lottie component in this package compared to the implementation that we have in the repo of our different projects (qonto-js, qonto-register-js etc).

In the current implementation we're using the buildWaiter function from the emberjs/ember-test-waiters addon in the loadLottie function to wait for async operations to complete before continuing tests. This allowed us NOT to add the waitFor test helper on the element of dom in tests when a Lottie is present.

The implementation in this package doesn't have this advantage and would otherwise force us, during migration, to go back over all the tests - and especially the percy snapshot - in all the repos to add waitFor's where needed and therefore add a new test rule which would state that if my test contains a Lottie, I must add a waitFor on the element of dom, even if we don't wish to assert that it's present.

What we would suggest here is to keep the buildWaiter logic in the animate function:

// addon/components/lottie.ts

import { buildWaiter } from '@ember/test-waiters';
...

const waiter = buildWaiter('ember-lottie:lottie-waiter');
...

export default class LottieComponent extends Component<LottieArgs> {
...

  @action
    async animate(element: HTMLElement): Promise<void> {
      const token = waiter.beginAsync(); <==
      const lottie = await this.loadLottie();
      let animationData;

      if (this.args.animationData) {
        animationData = this.args.animationData;
      } else if (this.args.path) {
        try {
          ...
        } catch (error) {
          ...
        } finally {
          waiter.endAsync(token); <==
        }
      }
      ...
    }
    ...
  }
}

WDYT?

SkoebaSteve commented 1 year ago

@clairekrucker seems reasonable to me, could you create a pull request for this?