search

MikeKovarik / gulp-better-rollup

📦 Better Gulp plugin for Rollup ES6 module bundler
MIT License
49 stars 16 forks source link

Simplify cache process. Fixes #42 #43

Closed halfnibble closed 5 years ago

halfnibble commented 5 years ago

I'm not sure how to write a test for this, but when using Map() for the cache, rollup eventually develops a memory leak and crashes. For me, it happens after 7 runs with gulp-watch.

This is obviously a problem with the Map implementation, but using a plain JavaScript object for the cache addresses the symptoms of the issue for me.

MikeKovarik commented 5 years ago

~Have you tried WeakMap?~ nevermind :D WeakMap doesn't work with string keys.

I'm not on my machine to try it out and I haven't dug through source of this lib in a long time. But as far as I remember and just by looking at the code. You didn't just replace Map with object, you restricted the plugin to just cache the last package. This won't work if you have mutliple rollup tasks in gulp.

instead of always replacing plugin's cache with rollupCache = bundle you would have to rollupCache[inputOptions.input] = bundle similarly to how it' now done with the Map.

halfnibble commented 5 years ago

Oh! Ok. I was wondering why this line existed: inputOptions.cache = rollupCache.get(inputOptions.input)

I think I can fix that.

halfnibble commented 5 years ago

I was somewhat concerned that using keys on an object would replicate my previous memory leak issue, but in my testing so far, this version with Objects and inputOptions.input key works great.

There must be something wrong with Map. Do you know where to file a bug report on Map for Node?