Resolve memory leak found by ember-cli-memory-leak-detector #118
Conversation
🦋 Changeset detectedLatest commit: c800b76 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Preview URLsGH Env: preview |
|
I don't know enough about pnpm to understand why it could cause hung tests but I guess this is a good opportunity to learn 🙃 |
NullVoxPopuli
force-pushed
the
add-memory-leak-detection
branch
from
6d3c9af to
dd99824
Compare
| @@ -10,6 +10,9 @@ module.exports = function (defaults) { | |||
| 'ember-cli-babel': { | |||
| enableTypeScriptTransform: true, | |||
| }, | |||
| 'ember-cli-memory-leak-detector': { | |||
| enabled: process.env.DETECT_MEMORY_LEAKS || false, | |||
There was a problem hiding this comment.
this is turned off by default, because of
- Causes tests to hang in PNPM repos steveszc/ember-cli-memory-leak-detector#51
- and Fix Qunit integration steveszc/ember-cli-memory-leak-detector#49
but the way to test for memory leaks is this:
Locally,
- build the package
- change the test-app's reference to the package to use the
fileprotocol, instead ofworkspace - change the test-app's qunit dependency to
~2.16.0and ember-qunit to^5.0.0 - delete node_modules for the test-app
- run yarn / npm in the test-app
- run
yarn ember test -s
There was a problem hiding this comment.
Might want to also note that you had to change the implementation in ember-cli-memory-leak-detector to gather the classes from the addon package, since out-of-the-box it only detects classes from the host app.
Also this PR is missing the testem.js config required to run memory-leak-detector.
There was a problem hiding this comment.
yeah, I had to remove everything, because there is still the hanging issue. But this is a good summary of what is needed to re-check for memory leaks (albeit, manually)
There was a problem hiding this comment.
Made a PR that aggregates all of this: #128 for reference
NullVoxPopuli
force-pushed
the
add-memory-leak-detection
branch
from
dd99824 to
c02d49d
Compare
NullVoxPopuli
changed the title
|
@steveszc @NullVoxPopuli I found a leak that should have been detected by *most of what I've learnt about memory leaks in ember, I've learnt from you @steveszc, so please feel free to close #266 if I have misunderstood the diagnostics. |
Locally, no errors 🎉