Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Memory leak fix #2273
Memory leak fix #2273
Conversation
|
we are waiting for the merge to happen so we can update our code base with yours, |
| @@ -60,8 +60,7 @@ | |||
| "node-sass": "^4.5.3", | |||
| "node-static": "^0.7.9", | |||
| "nodemon": "^1.11.0", | |||
| "rollup": "^0.55.0", | |||
kt3k
Feb 1, 2018
Member
I think rollup is necessary to build the bundled version (c3.js).
I think rollup is necessary to build the bundled version (c3.js).
nazihahmed
Feb 1, 2018
Author
Contributor
It must be global because in the scripts where it's called you are using the global rollup command not from node modules, anyways i will add it back
It must be global because in the scripts where it's called you are using the global rollup command not from node modules, anyways i will add it back
| @@ -0,0 +1,13 @@ | |||
| { | |||
kt3k
Feb 1, 2018
Member
Because babel is configured in rollup.config.js (when bundling) and karma.conf.js (when testing), I think this file is not necessary and confusing.
Because babel is configured in rollup.config.js (when bundling) and karma.conf.js (when testing), I think this file is not necessary and confusing.
nazihahmed
Feb 1, 2018
Author
Contributor
For me the build keeps returning an error from rollup until i added this file
For me the build keeps returning an error from rollup until i added this file
kt3k
Feb 1, 2018
Member
Do you use npm run command for invoking rollup?
npm run build command should invoke rollup (and other tools) with correct configs, like:
$ npm run build
> c3@0.4.18 build /Users/kt3k/t/c3
> npm run build:js && npm run build:css
> c3@0.4.18 build:js /Users/kt3k/t/c3
> npm run build:js:rollup && npm run build:js:uglify
> c3@0.4.18 build:js:rollup /Users/kt3k/t/c3
> rollup -c > c3.js
(!) Some options have been renamed
https://gist.github.com/Rich-Harris/d472c50732dab03efeb37472b08a3f32
entry is now input
moduleName is now output.name
format is now output.format
...
npm run command automatically uses local rollup cli and you don't need the global one.
Do you use npm run command for invoking rollup?
npm run build command should invoke rollup (and other tools) with correct configs, like:
$ npm run build
> c3@0.4.18 build /Users/kt3k/t/c3
> npm run build:js && npm run build:css
> c3@0.4.18 build:js /Users/kt3k/t/c3
> npm run build:js:rollup && npm run build:js:uglify
> c3@0.4.18 build:js:rollup /Users/kt3k/t/c3
> rollup -c > c3.js
(!) Some options have been renamed
https://gist.github.com/Rich-Harris/d472c50732dab03efeb37472b08a3f32
entry is now input
moduleName is now output.name
format is now output.format
...npm run command automatically uses local rollup cli and you don't need the global one.
nazihahmed
Feb 1, 2018
Author
Contributor
yes, I do use this, but the issue is not rollup, rollup works but throws a different error (It looks like your Babel configuration specifies a module transformer), so I added this file and it worked for me, anyways I removed this file for you to merge.
when will you publish the next version to npm? we are still using our own library.
yes, I do use this, but the issue is not rollup, rollup works but throws a different error (It looks like your Babel configuration specifies a module transformer), so I added this file and it worked for me, anyways I removed this file for you to merge.
when will you publish the next version to npm? we are still using our own library.
|
Thanks for finding out these! |
Codecov Report
@@ Coverage Diff @@
## master #2273 +/- ##
==========================================
+ Coverage 73.88% 75.39% +1.51%
==========================================
Files 51 51
Lines 4185 4186 +1
==========================================
+ Hits 3092 3156 +64
+ Misses 1093 1030 -63
Continue to review full report at Codecov.
|
|
is there any update on this? Would love to see it go into master. |
|
@Mobiletainment you can use alooma-c3 while they release the new version, after they release it you simply change back to c3 in your package.json |
|
The root problem was that the attached function (resizeIfElementDisplayed) and the detached one (resizeFunction) were different and the attached one was leaked every time. This seems to have started at #2164. Thanks again for finding out this! By the way $$.resizeFunction.remove() doesn't seem removing internal resize functions, because it removes the only given function (see Lines 988 to 995 in 7e5c20c |
|
@kt3k great, this is the problem that I solved, hopefully not introducing another one.. |
the detach event listener was not detached properly after calling .destroy
so I basically added resizeIfElementDisplayed to $$ (this), and detach the event listener properly
also .remove must be called on $$.resizeFunction on destroy,
anyways this fixed all my memory issue,
Fix #926