Skip to content
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.

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

moduleForComponent does not properly destroy the views it created #64

Closed
chancancode opened this issue Jun 6, 2014 · 4 comments
Closed

Comments

@chancancode
Copy link
Member

I believe this is what this TODO comment is about. This just bit me and caused a few hours of pulling hair :)

The scenario I have is that my component attaches a click callback on the body on didInsertElement (to dismiss itself when clicked outside), which is removed in willDestroyElement. Problem is, since the views are not actually destroyed, the willDestroyElement hooks and friends are never fired.

So, a few (completely unrelated) test cases later, I used the click helper somewhere and the whole thing blew up with a Error: Cannot perform operations on a Metamorph that is not in the DOM.. It was an epic WTF moment 😅

This might also explain why the tests are leaking memory 😉

@chancancode
Copy link
Member Author

A quick patch:

diff --git a/test/javascripts/support/ember-qunit.js b/test/javascripts/support/ember-qunit.js
index 4ffa73f..ddacede 100644
--- a/test/javascripts/support/ember-qunit.js
+++ b/test/javascripts/support/ember-qunit.js
@@ -70,12 +70,11 @@ exports["default"] = function moduleForComponent(name, description, callbacks) {
     context.dispatcher.setup({}, '#ember-testing');

     context.__setup_properties__.render = function() {
-      var containerView = Ember.ContainerView.create({container: container});
+      context.__container_view__ = Ember.ContainerView.create({container: container});
       var view = Ember.run(function(){
         var subject = context.subject();
-        containerView.pushObject(subject);
-        // TODO: destory this somewhere
-        containerView.appendTo('#ember-testing');
+        context.__container_view__.pushObject(subject);
+        context.__container_view__.appendTo('#ember-testing');
         return subject;
       });

@@ -179,6 +178,10 @@ exports["default"] = function moduleFor(fullName, description, callbacks, delega

     teardown: function(){
       Ember.run(function(){
+        if (context.__container_view__) {
+          context.__container_view__.destroy();
+        }
+
         container.destroy();

         if (context.dispatcher) {

It seems to have addressed the issue for me. It does have some side-effect though. It now triggers the focusout event on the elements as they are being torn down, which broke my tests. But that's probably not ember-qunit problem.

@stefanpenner
Copy link
Member

@chancancode wanna submit a PR?

@chancancode
Copy link
Member Author

Is this patch good enough? Or are we looking for a more generic solution?

@chancancode
Copy link
Member Author

There is an additional "problem" with this:

In a component, I have on('didInsertElement') to hook up a jquery plugin, and I clean that up in on('willDestroyElement'). Problem is, with this patch, occasionally the clean up code will be called during tear down, but the setup code was never called, causing the jquery plugin to blow up. I'm still not entirely sure how this is possible (because the container view shouldn't have been created in the first place). Need to investigate more to find out why.

gordonkristan added a commit to gordonkristan/ember-qunit that referenced this issue Aug 14, 2014
@rwjblue rwjblue closed this as completed in d38c870 Sep 5, 2014
Turbo87 pushed a commit that referenced this issue Oct 13, 2018
Bumps [broccoli-merge-trees](https://github.com/broccolijs/broccoli-merge-trees) from 2.0.1 to 3.0.1.
<details>
<summary>Changelog</summary>

*Sourced from [broccoli-merge-trees's changelog](https://github.com/broccolijs/broccoli-merge-trees/blob/master/CHANGELOG.md).*

> # master
> 
> # 3.0.0
> 
> * Upgrade to node-merge-trees ^2.0.0; this reduces code complexity and fixes an
>   obscure bug
> 
> * Bump minimum Node version to 6.0.0
> 
> # 2.0.0
> 
> * Require Node >= 4
> 
> # 1.2.4
> 
> * Revert to version 1.2.1 to restore Node 0.10 compatibility
> 
> # 1.2.3
> 
> * Bump merge-trees dependency for Node 4 compatibility
> 
> # 1.2.2
> 
> * Extract logic into node-merge-trees package
> 
> # 1.2.1
> 
> * Fix typo with cache directory opt-out.
> 
> # 1.2.0
> 
> * Opt out of cache directory creation.
> 
> # 1.1.4
> 
> * Use heimdalljs for instrumentation and logging
> 
> # 1.1.3
> 
> * Include only necessary files in npm package
> 
> # 1.1.2
> 
> * Update fs-tree-diff, fixes "rename only file in directory" bug, and possible performance improvements.
> 
> # 1.1.1
> 
> * Upgrade to can-symlink 1.0.0
> 
> # 1.1.0
></table> ... (truncated)
</details>
<details>
<summary>Commits</summary>

- [`f588e5f`](broccolijs/broccoli-merge-trees@f588e5f) release v3.0.1 🎉
- [`a7df138`](broccolijs/broccoli-merge-trees@a7df138) Merge pull request [#64](https://github-redirect.dependabot.com/broccolijs/broccoli-merge-trees/issues/64) from broccolijs/hjdivad/merge-trees-err-msg
- [`d701be6`](broccolijs/broccoli-merge-trees@d701be6) build adds input nodes to error message
- [`35b63e5`](broccolijs/broccoli-merge-trees@35b63e5) Update devDependencies
- [`b5985e2`](broccolijs/broccoli-merge-trees@b5985e2) Do not claim the new version is faster; it seems to be within the margin of e...
- [`24643af`](broccolijs/broccoli-merge-trees@24643af) Clarify CHANGELOG entry
- [`98187b1`](broccolijs/broccoli-merge-trees@98187b1) Release version 3.0.0
- [`7b6f8c2`](broccolijs/broccoli-merge-trees@7b6f8c2) Update .travis.yml and appveyor.yml
- [`545a157`](broccolijs/broccoli-merge-trees@545a157) Upgrade to node-merge-trees ^2.0.0; this requires Node 6.0.0 or greater
- See full diff in [compare view](broccolijs/broccoli-merge-trees@v2.0.1...v3.0.1)
</details>
<br />

[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=broccoli-merge-trees&package-manager=npm_and_yarn&previous-version=2.0.1&new-version=3.0.1)](https://dependabot.com/compatibility-score.html?dependency-name=broccoli-merge-trees&package-manager=npm_and_yarn&previous-version=2.0.1&new-version=3.0.1)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

Dependabot will **not** automatically merge this PR because it includes an out-of-range update to a production dependency.

---

**Note:** This repo was added to Dependabot recently, so you'll receive a maximum of 5 PRs for your first few update runs. Once an update run creates fewer than 5 PRs we'll remove that limit.

You can always request more updates by clicking `Bump now` in your [Dependabot dashboard](https://app.dependabot.com).

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot ignore this [patch|minor|major] version` will close this PR and stop Dependabot creating any more for this minor/major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language
- `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language
- `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language
- `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com):
- Update frequency (including time of day and day of week)
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Pull request limits (per update run and/or open at any time)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)

Finally, you can contact us by mentioning @dependabot.

</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants