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

Banner string duplicates on watchify rebundle #1

Open
cguinnup opened this issue Nov 7, 2017 · 4 comments
Open

Banner string duplicates on watchify rebundle #1

cguinnup opened this issue Nov 7, 2017 · 4 comments

Comments

@cguinnup
Copy link

cguinnup commented Nov 7, 2017

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
Copy link
Author

cguinnup commented Nov 7, 2017

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

@JamesMessinger
Copy link
Member

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
Copy link
Author

cguinnup commented Nov 7, 2017

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants