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

Fix for factor-bundle + watchify + gulp 'EventEmitter memory leak detected' issue #65

Closed
wants to merge 1 commit into from

Conversation

ypt
Copy link

@ypt ypt commented Apr 24, 2015

Implemented the fix for #64 suggested by @Freyert

@rtibbles
Copy link

Could this be merged? Pretty handy to have it in master.

@d0b1010r
Copy link

+1

@Freyert
Copy link

Freyert commented Sep 9, 2015

#proud

@zoubin
Copy link

zoubin commented Sep 10, 2015

Right now you can specify opts.outputs as a function, and thus no need to plugin factor-bundle each time reset.

I have tried some way to use watchify with factor-bundle, no memory leak detected.

I think plugging factor-bundle repeatedly seems a little odd, because reset only rebuild the pipeline, and all other plugins are plugged only once.

@davidchase
Copy link

@zoubin you are correct i tired it out by moving factor-bundle outside of my re-bundling method and it works therefore this fix is un-needed at least for me as well.

my simple setup using browserify-incremental + chokidar

var bundleStream = browserifyInc(files, {
    cache: {},
    packageCache: {},
    fullPaths: true,
    cacheFile: 'browserify-cache.json'
}).plugin('factor-bundle', {
    outputs: outputFiles
});

var bundleScripts = function bundleScripts() {
    bundleStream
        .bundle()
        .pipe(fs.createWriteStream('./client/dist/js/common.js'));
};

@rtibbles
Copy link

I think this may be my issue as well.

@ypt
Copy link
Author

ypt commented Oct 20, 2015

Alright, it sounds like this fix isn't really necessary, and @zoubin and @davidchase have solutions. I'll close this PR.

@ypt ypt closed this Oct 20, 2015
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

Successfully merging this pull request may close these issues.

6 participants