qonto / ember-lottie

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

fix: end test waiter when using @animationData arg #441

Closed 22a closed 7 months ago

22a commented 8 months ago

This PR ends the test waiter (inside the <Lottie> component's animate action) after reading the @animationData arg.

Prior to this change, the test waiter is started when using @path or @animationData: https://github.com/qonto/ember-lottie/blob/144518e964684fd3b99fd6efd4a87f7d7ce3b07f/ember-lottie/src/components/lottie.ts#L68

but it was only ended when using @path - so specs relying on @animationData hang and timeout: https://github.com/qonto/ember-lottie/blob/144518e964684fd3b99fd6efd4a87f7d7ce3b07f/ember-lottie/src/components/lottie.ts#L94

Testing

I have patch-package'd this change into our app and can confirm it works as expected:

Screenshot of passing spec after patch failed import:

![Screenshot of passing spec after applying the patch in this PR](https://github.com/qonto/ember-lottie/assets/7144173/2386a47b-7570-4610-89a4-171b3cd82d55)

I attempted to write a failing spec to reproduce the @animationData test waiter bug but couldn't find an ergonomic way to provide a reasonable @animationData payload without checking in a big bag of JSON. In this branch I tried to reach up into the public/ directory where you already have a suitable json payload but I ran into import issues (see details below) and it already involved fiddling with the test app's tsconfig.json so I parked it. If you're okay with me tweaking tsconfig and duplicating test-app/public/data.json somewhere under app/ I'm happy to prep a PR to add a suitable spec.

Details on test-app/public/data.json failed import

``` [test:ember] not ok 1 Chrome 122.0 - [1 ms] - TestLoader Failures: test-app/tests/integration/components/lottie-test: could not be loaded │ [test:ember] --- │ [test:ember] actual: > │ [test:ember] null │ [test:ember] stack: > │ [test:ember] Error: Could not find module `test-app/public/data.json` imported from `test-app/tests/integration/components/lottie-test` │ [test:ember] at missingModule (http://localhost:7357/assets/vendor.js:266:11) │ [test:ember] at findModule (http://localhost:7357/assets/vendor.js:277:7) │ [test:ember] at Module.findDeps (http://localhost:7357/assets/vendor.js:187:24) │ [test:ember] at findModule (http://localhost:7357/assets/vendor.js:281:11) │ [test:ember] at requireModule (http://localhost:7357/assets/vendor.js:43:15) │ [test:ember] at TestLoader.require (http://localhost:7357/assets/test-support.js:6981:9) │ [test:ember] at TestLoader.loadModules (http://localhost:7357/assets/test-support.js:6975:14) │ [test:ember] at loadTests (http://localhost:7357/assets/chunk.vendors-node_modules_pnpm_ember-qunit_8_0_2__ember_test-helpers_3_3_0__glint_template_1_3_0_e-8ce205.7e0dc8cc4603a627299a.js:320:42) │ [test:ember] at start (http://localhost:7357/assets/chunk.vendors-node_modules_pnpm_ember-qunit_8_0_2__ember_test-helpers_3_3_0__glint_template_1_3_0_e-8ce205.7e0dc8cc4603a627299a.js:214:119) │ [test:ember] at Module.callback (http://localhost:7357/assets/tests.js:395:25) ```

22a commented 7 months ago

Hey @anas7asia, is there anything you'd like me to change about this PR?

oreqizer commented 7 months ago

added a test here https://github.com/22a/ember-lottie/compare/main...oreqizer:ember-lottie:main

I reverted the change and the test indeed failed. adding this commit's change fixes the test 👍

anas7asia commented 7 months ago

Hello @22a! Thank you for the changes you've proposed. Let's merge https://github.com/22a/ember-lottie/pull/1#pullrequestreview-1996624612 proposed by @oreqizer first to your fork and then merge this PR 🙌

22a commented 7 months ago

Excellent, thank you @oreqizer @anas7asia - https://github.com/22a/ember-lottie/pull/1 has been merged

anas7asia commented 7 months ago

Thank you @22a, can you squash your commits before we merge, please?

22a commented 7 months ago

@anas7asia sure thing, done 👍

oreqizer commented 7 months ago

Hey @22a, the lint job was timing out because of the new fixtures/data.ts file. Apparently, /* eslint-disable */ is not enough there.

This PR should address that by ignoring fixtures in .eslintignore. 🙏

CC @anas7asia

22a commented 7 months ago

nice one @oreqizer - merged and squashed again

22a commented 7 months ago

apologies, my previous commit violated your commit message linting. fixed.

oreqizer commented 7 months ago

failing job is unrelated, merging

anas7asia commented 6 months ago

@22a, the fix is available in the latest release: https://github.com/qonto/ember-lottie/releases/tag/v1.1.3 Thank you again for submitting your PR! ❤️

cc @oreqizer 🙏