JS-DevTools / browserify-banner

Add a banner (comment and/or code) to the top of your Browserify bundle
https://jstools.dev/browserify-banner/
MIT License
9 stars 1 forks source link

Banner string duplicates on watchify rebundle #1

Open cguinnup opened 6 years ago

cguinnup commented 6 years ago

Thanks for the plugin, I especially appreciate the source map support.

I'm adding a simple string banner and performing incremental rebundles with watchify. On the first bundling, everything is correct. After changing source code & triggering subsequent rebundles, there are suddenly duplicate instances of the banner string. This duplication causes an error when the script runs.

I believe my use of browserify-banner is correct:

let scriptBundler = browserify({
    entries: entry + '.js',
    basedir: buildDir,
    debug: devMode,  // Source maps!
    cache: {},         // For watchify's incremental bundling 
    packageCache: {},  // "
    fullPaths: false,  // "
    noParse: ['jquery'], // No require() in this big lib: save time & don't parse
    browserField: false,              // Makes bundle runnable in Node
    builtins: false,                  // "
    commondir: false,                 // "
    insertGlobalVars: {               // "
        Buffer: undefined,            // "
        'Buffer.isBuffer': undefined, // "
        global: undefined,            // "
        process: undefined            // "
    },                               
});

// Make the Node script runnable in the shell
let scriptHeader = '#!/usr/bin/env node\n';
if (devMode) {
    // Make Node show source code in stack traces
    scriptHeader += 'require("source-map-support").install();\n';
    scriptBundler.plugin(watchify);
}
scriptBundler.plugin(banner, {banner: scriptHeader});

// The rebundle process
const rebundle = function () {
    var start = Date.now();
    console.log('Bundling "' + entry + '"');
    return scriptBundler.bundle()
      .pipe(vinylStream(entry))
      .pipe(gulpif(!devMode && !argv['skip-minify'], vinylBuffer()))
      .pipe(gulpif(!devMode && !argv['skip-minify'], uglify()))
      .pipe(chmod({ execute: true }))
      .pipe(gulp.dest(distDir))
      .pipe(notify(function() {
        console.log('Bundled  "' + entry + '" in ' + (Date.now() - start) / 1000 + ' s');
      }));
};

// Watch for changes while developing
if (devMode) {
    scriptBundler.on('update', rebundle);
}
cguinnup commented 6 years ago

Removing browserify-banner's line 32 browserify.on('reset', wrapBundle); seems to fix the issue

JamesMessinger commented 6 years ago

Hi. Thanks for opening this issue. I don't use watchify very often, so I've never run into this myself. But your analysis makes sense.

I know that the "reset" event is needed in some cases, so I don't feel comfortable completely removing it. Maybe we can add logic to detect if it's being run in a watchify process and, if so, then not listen for that event.

cguinnup commented 6 years ago

According to Browserify docs, the reset event seems to be called between multiple bundle() invocations. Since your bundle() invocation already prepends the banner, it doesn't appear that a reset event should be necessary, unless you have cleanup to perform in between bundlings.

JamesMessinger commented 6 years ago

Hmmm... ok. It's possible that I'm wrong and the "reset" event is entirely unnecessary. Do all the tests still pass if you remove line 32? If so, then I'll gladly accept a PR.