DockYard / ember-in-viewport

Detect if an Ember View or Component is in the viewport @ 60FPS
MIT License
245 stars 91 forks source link

Fix test failures in Ember 3.20: Run service teardown code in willDestroy() #241

Closed bendemboski closed 4 years ago

bendemboski commented 4 years ago

Put the service teardown code in willDestroy() instead of destroy() so it runs at the expected time. destroy() schedules a call to willDestroy() on the run loop, so they happen at different times, and willDestroy() is the hook where teardown code is expected to live. Starting in Ember 3.20 having this code in destroy() can cause test teardown bugs if a modifier calls into the service to stop watching, because things happen in this order:

  1. Service's destroy() method is called (by the container)
  2. Modifiers' willRemove() hooks are called
  3. Service's willDestroy() hook is called

So the service's teardown code needs to live in willDestroy() or else it will have already torn down by step (2) and calls into it from modifier teardown code can cause errors.

Note that I did not add any new tests as part of this PR because the existing tests are failing in the beta and (I'm pretty sure) canary scenarios, so this change already has test coverage.

Closes #240

makepanic commented 4 years ago

Can confirm, this change fixes our build breaking with ember-in-viewport and ember-source@3.20

bendemboski commented 4 years ago

The destroy() method sets the isDestroying flag and schedules a callback on the destroy queue, and that callback calls the willDestroy hook (if implemented) and then does the actual work to destroy the object. willDestroy is where we're meant to do custom teardown work.