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

Polyfill memory leak prevention. #328

Merged
merged 1 commit into from
May 7, 2018
Merged

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented May 4, 2018

This polyfills the changes from qunitjs/qunit#1279 to older versions of QUnit.


Prior to the changes in this PR, all module and test callbacks are retained (forever). This may not seem significant, but as folks use closure scope to store data across tests (which is very common).

For example, prior to the changes in this PR the following would retain the local variable:

QUnit.module('top', function(hooks) {
  let largeThing;

  hooks.beforeEach(function() {
    largeThing = new LargeThing();
  });

  hooks.afterEach(function() {
    largeThing.destroy();
  });

  test('something that uses largeThing', function(assert) {
    // ...snip...
    largeThing.someMethod();
  });
});

In larger test suites, this really adds up...

Copy link
Member

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assuming you've tested this on a real world project, this seems good to me 👍

@rwjblue
Copy link
Member Author

rwjblue commented May 7, 2018

assuming you've tested this on a real world project

Confirm, I've tested in a few different apps and also confirmed that prior known leaks (like the one in the description) are resolved.

@rwjblue rwjblue merged commit 845fba7 into master May 7, 2018
@rwjblue rwjblue deleted the polyfill-memory-leak-prevention branch May 7, 2018 13:34
@rwjblue rwjblue added the bug label May 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants