handles tricky consistent renaming cases

src/Bundle/index.js

@@ -1,8 +1,9 @@
 import { resolve, sep } from 'path';
 import { readFile } from 'sander';
 import MagicString from 'magic-string';
-import { hasOwnProp } from '../utils/object';
+import { keys, has } from '../utils/object';
 import { sequence } from '../utils/promise';
+import sanitize from '../utils/sanitize';
 import Module from '../Module/index';
 import finalisers from '../finalisers/index';
 import replaceIdentifiers from '../utils/replaceIdentifiers';
@@ -15,12 +16,13 @@ export default class Bundle {
 
 		this.modulePromises = {};
 		this.modules = {};
+		this.modulesArray = [];
 
 		// this will store the top-level AST nodes we import
 		this.body = [];
 
 		// this will store per-module names, and enable deconflicting
-		this.names = {};
+		this.bindingNames = {};
 		this.usedNames = {};
 
 		this.externalModules = [];
@@ -34,7 +36,7 @@ export default class Bundle {
 	}
 
 	fetchModule ( path ) {
-		if ( !hasOwnProp.call( this.modulePromises, path ) ) {
+		if ( !has( this.modulePromises, path ) ) {
 			this.modulePromises[ path ] = readFile( path, { encoding: 'utf-8' })
 				.then( code => {
 					const module = new Module({
@@ -43,7 +45,16 @@ export default class Bundle {
 						bundle: this
 					});
 
+					//const bindingNames = bundle.getBindingNamesFor( module );
+
+					// we need to ensure that this module's top-level
+					// declarations don't conflict with the bundle so far
+					module.definedNames.forEach( name => {
+
+					});
+
 					this.modules[ path ] = module;
+					this.modulesArray.push( module );
 					return module;
 				});
 		}
@@ -51,14 +62,30 @@ export default class Bundle {
 		return this.modulePromises[ path ];
 	}
 
+	getBindingNamesFor ( module ) {
+		if ( !has( this.bindingNames, module.path ) ) {
+			this.bindingNames[ module.path ] = {};
+		}
+
+		return this.bindingNames[ module.path ];
+	}
+
 	build () {
 		// bring in top-level AST nodes from the entry module
 		return this.fetchModule( this.entryPath )
 			.then( entryModule => {
 				this.entryModule = entryModule;
 
+				const importedNames = keys( entryModule.imports );
+
+				entryModule.definedNames
+					.concat( importedNames )
+					.forEach( name => {
+						this.usedNames[ name ] = true;
+					});
+
 				// pull in imports
-				return sequence( Object.keys( entryModule.imports ), name => {
+				return sequence( importedNames, name => {
 					return entryModule.define( name )
 						.then( nodes => {
 							this.body.push.apply( this.body, nodes );
@@ -72,7 +99,58 @@ export default class Bundle {
 							}
 						});
 					});
+			})
+			.then( () => {
+				this.deconflict();
 			});
+
+	}
+
+	deconflict () {
+		let definers = {};
+		let conflicts = {};
+
+		this.body.forEach( statement => {
+			keys( statement._defines ).forEach( name => {
+				if ( has( definers, name ) ) {
+					conflicts[ name ] = true;
+				} else {
+					definers[ name ] = [];
+				}
+
+				// TODO in good js, there shouldn't be duplicate definitions
+				// per module... but some people write bad js
+				definers[ name ].push( statement._module );
+			});
+		});
+
+		keys( conflicts ).forEach( name => {
+			const modules = definers[ name ];
+
+			modules.pop(); // the module closest to the entryModule gets away with keeping things as they are
+
+			modules.forEach( module => {
+				module.rename( name, name + '$' + ~~( Math.random() * 100000 ) ); // TODO proper deconfliction mechanism
+			});
+		});
+
+		this.body.forEach( statement => {
+			let replacements = {};
+
+
+
+			keys( statement._dependsOn )
+				.concat( keys( statement._defines ) )
+				.forEach( name => {
+					const canonicalName = statement._module.getCanonicalName( name );
+
+					if ( name !== canonicalName ) {
+						replacements[ name ] = canonicalName;
+					}
+				});
+
+			replaceIdentifiers( statement, statement._source, replacements );
+		});
 	}
 
 	generate ( options = {} ) {
@@ -88,7 +166,7 @@ export default class Bundle {
 		const finalise = finalisers[ options.format || 'es6' ];
 
 		if ( !finalise ) {
-			throw new Error( `You must specify an output type - valid options are ${Object.keys( finalisers ).join( ', ' )}` );
+			throw new Error( `You must specify an output type - valid options are ${keys( finalisers ).join( ', ' )}` );
 		}
 
 		magicString = finalise( this, magicString, options );
@@ -100,4 +178,40 @@ export default class Bundle {
 			})
 		};
 	}
+
+	getSafeReplacement ( name, requestingModule ) {
+		// assume name is safe until proven otherwise
+		let safe = true;
+
+		name = sanitize( name );
+
+		let pathParts = requestingModule.relativePath.split( sep );
+
+		do {
+			let safe = true;
+
+			let i = this.modulesArray.length;
+			while ( safe && i-- ) {
+				const module = this.modulesArray[i];
+				if ( module === requestingModule ) continue;
+
+				let j = module.definedNames.length;
+				while ( safe && j-- ) {
+					if ( module.definedNames[j] === name ) {
+						safe = false;
+					}
+				}
+			}
+
+			if ( !safe ) {
+				if ( pathParts.length ) {
+					name = sanitize( pathParts.pop() ) + `__${name}`;
+				} else {
+					name = `_${name}`;
+				}
+			}
+		} while ( !safe );
+
+		return name;
+	}
 }

src/Module/index.js

@@ -3,7 +3,7 @@ import { Promise } from 'sander';
 import { parse } from 'acorn';
 import MagicString from 'magic-string';
 import analyse from '../ast/analyse';
-import { hasOwnProp } from '../utils/object';
+import { has } from '../utils/object';
 import { sequence } from '../utils/promise';
 
 const emptyArrayPromise = Promise.resolve([]);
@@ -20,9 +20,18 @@ export default class Module {
 			sourceType: 'module'
 		});
 
+		this.analyse();
+
+		this.deconflict();
+	}
+
+	analyse () {
 		analyse( this.ast, this.code, this );
 
+		this.definedNames = this.ast._scope.names.slice();
+
 		this.nameReplacements = {};
+		this.canonicalNames = {};
 
 		this.definitions = {};
 		this.definitionPromises = {};
@@ -34,7 +43,7 @@ export default class Module {
 			});
 
 			Object.keys( statement._modifies ).forEach( name => {
-				if ( !hasOwnProp.call( this.modifications, name ) ) {
+				if ( !has( this.modifications, name ) ) {
 					this.modifications[ name ] = [];
 				}
 
@@ -46,6 +55,8 @@ export default class Module {
 		this.exports = {};
 
 		this.ast.body.forEach( node => {
+			// import foo from './foo';
+			// import { bar } from './bar';
 			if ( node.type === 'ImportDeclaration' ) {
 				const source = node.source.value;
 
@@ -61,6 +72,9 @@ export default class Module {
 				});
 			}
 
+			// export default function foo () {}
+			// export default foo;
+			// export default 42;
 			else if ( node.type === 'ExportDefaultDeclaration' ) {
 				const isDeclaration = /Declaration$/.test( node.declaration.type );
 
@@ -68,151 +82,214 @@ export default class Module {
 					node,
 					name: 'default',
 					localName: isDeclaration ? node.declaration.id.name : 'default',
-					isDeclaration
+					isDeclaration,
+					module: null // filled in later
 				};
 			}
 
+			// export { foo, bar, baz }
+			// export var foo = 42;
+			// export function foo () {}
 			else if ( node.type === 'ExportNamedDeclaration' ) {
-				let declaration = node.declaration;
+				if ( node.specifiers.length ) {
+					// export { foo, bar, baz }
+					node.specifiers.forEach( specifier => {
+						const localName = specifier.local.name;
+						const exportedName = specifier.exported.name;
+
+						this.exports[ exportedName ] = {
+							localName
+						};
+					});
+				}
+
+				else {
+					let declaration = node.declaration;
 
-				if ( declaration ) {
 					let name;
 
 					if ( declaration.type === 'VariableDeclaration' ) {
-						// `export var foo = /*...*/`
+						// export var foo = 42
 						name = declaration.declarations[0].id.name;
 					} else {
-						// `export function foo () {/*...*/}`
+						// export function foo () {}
 						name = declaration.id.name;
 					}
 
 					this.exports[ name ] = {
 						localName: name,
-						expression: node.declaration
+						expression: declaration
 					};
 				}
+			}
+		});
+	}
 
-				else if ( node.specifiers ) {
-					node.specifiers.forEach( specifier => {
-						const localName = specifier.local.name;
-						const exportedName = specifier.exported.name;
+	getCanonicalName ( name ) {
+		if ( has( this.imports, name ) ) {
+			const importDeclaration = this.imports[ name ];
+			const module = importDeclaration.module;
+			const exportDeclaration = module.exports[ importDeclaration.name ];
 
-						this.exports[ exportedName ] = {
-							localName
-						};
-					});
-				}
+			return module.getCanonicalName( exportDeclaration.localName );
+		}
+
+		if ( name === 'default' ) {
+			name = this.defaultExportName;
+		}
+
+		return has( this.canonicalNames, name ) ? this.canonicalNames[ name ] : name;
+	}
+
+	deconflict () {
 
-			}
-		});
 	}
 
 	define ( name ) {
 		// shortcut cycles. TODO this won't work everywhere...
-		if ( hasOwnProp.call( this.definitionPromises, name ) ) {
+		if ( has( this.definitionPromises, name ) ) {
 			return emptyArrayPromise;
 		}
 
-		if ( !hasOwnProp.call( this.definitionPromises, name ) ) {
-			let promise;
+		let promise;
 
-			// The definition for this name is in a different module
-			if ( hasOwnProp.call( this.imports, name ) ) {
-				const importDeclaration = this.imports[ name ];
-				const path = resolve( dirname( this.path ), importDeclaration.source ) + '.js';
+		// The definition for this name is in a different module
+		if ( has( this.imports, name ) ) {
+			const importDeclaration = this.imports[ name ];
+			const path = resolve( dirname( this.path ), importDeclaration.source ) + '.js';
 
-				promise = this.bundle.fetchModule( path )
-					.then( module => {
-						const exportDeclaration = module.exports[ importDeclaration.name ];
+			promise = this.bundle.fetchModule( path )
+				.then( module => {
+					importDeclaration.module = module;
 
-						if ( !exportDeclaration ) {
-							throw new Error( `Module ${module.path} does not export ${importDeclaration.name} (imported by ${this.path})` );
-						}
+					const exportDeclaration = module.exports[ importDeclaration.name ];
 
-						const globalName = module.nameReplacements[ exportDeclaration.localName ];
-						if ( globalName ) {
-							this.rename( importDeclaration.localName, globalName );
-						} else {
-							module.rename( exportDeclaration.localName, importDeclaration.localName );
-						}
+					if ( !exportDeclaration ) {
+						throw new Error( `Module ${module.path} does not export ${importDeclaration.name} (imported by ${this.path})` );
+					}
 
-						return module.define( exportDeclaration.localName );
-					});
-			}
+					if ( importDeclaration.name === 'default' ) {
+						module.suggestDefaultName( importDeclaration.localName );
+					}
 
-			// The definition is in this module
-			else if ( name === 'default' && this.exports.default.isDeclaration ) {
-				// We have something like `export default foo` - so we just start again,
-				// searching for `foo` instead of default. First, sync up names
-				this.rename( 'default', this.exports.default.name );
-				promise = this.define( this.exports.default.name );
-			}
+					// const globalName = module.nameReplacements[ exportDeclaration.localName ];
+					// if ( globalName ) {
+					// 	this.rename( importDeclaration.localName, globalName, true );
+					// } else {
+					// 	module.rename( exportDeclaration.localName, importDeclaration.localName );
+					// }
 
-			else {
-				let statement;
+					return module.define( exportDeclaration.localName );
+				});
+		}
 
-				if ( name === 'default' ) {
-					// We have an expression, e.g. `export default 42`. We have
-					// to assign that expression to a variable
-					const replacement = this.nameReplacements.default;
+		// The definition is in this module
+		else if ( name === 'default' && this.exports.default.isDeclaration ) {
+			// We have something like `export default foo` - so we just start again,
+			// searching for `foo` instead of default. First, sync up names
+			this.rename( 'default', this.exports.default.name );
+			promise = this.define( this.exports.default.name );
+		}
 
-					statement = this.exports.default.node;
+		else {
+			let statement;
 
-					if ( !statement._imported ) {
-						statement._source.overwrite( statement.start, statement.declaration.start, `var ${replacement} = ` )
+			if ( name === 'default' ) {
+				// We have an expression, e.g. `export default 42`. We have
+				// to assign that expression to a variable
+				const replacement = this.defaultExportName;
+
+				statement = this.exports.default.node;
+
+				if ( !statement._imported ) {
+					// if we have `export default foo`, we don't want to turn it into `var foo = foo`
+					// - we want to remove it altogether (but keep the statement, so we can include
+					// its dependencies). TODO is there an easier way to do this?
+					const shouldRemove = statement.declaration.type === 'Identifier' && statement.declaration.name === replacement;
+
+					if ( shouldRemove ) {
+						statement._source.remove( statement.start, statement.end );
+					} else {
+						statement._source.overwrite( statement.start, statement.declaration.start, `var ${replacement} = ` );
 					}
 				}
+			}
 
-				else {
-					statement = this.definitions[ name ];
+			else {
+				statement = this.definitions[ name ];
 
-					if ( statement && /^Export/.test( statement.type ) ) {
-						statement._source.remove( statement.start, statement.declaration.start );
-					}
+				if ( statement && /^Export/.test( statement.type ) ) {
+					statement._source.remove( statement.start, statement.declaration.start );
 				}
+			}
 
-				if ( statement && !statement._imported ) {
-					const nodes = [];
+			if ( statement && !statement._imported ) {
+				const nodes = [];
 
-					const include = statement => {
-						if ( statement._imported ) return emptyArrayPromise;
+				const include = statement => {
+					if ( statement._imported ) return emptyArrayPromise;
 
-						const dependencies = Object.keys( statement._dependsOn );
+					const dependencies = Object.keys( statement._dependsOn );
 
-						return sequence( dependencies, name => this.define( name ) )
-							.then( definitions => {
-								definitions.forEach( definition => nodes.push.apply( nodes, definition ) );
-							})
-							.then( () => {
-								statement._imported = true;
-								nodes.push( statement );
+					return sequence( dependencies, name => this.define( name ) )
+						.then( definitions => {
+							definitions.forEach( definition => nodes.push.apply( nodes, definition ) );
+						})
+						.then( () => {
+							statement._imported = true;
+							nodes.push( statement );
 
-								const modifications = hasOwnProp.call( this.modifications, name ) && this.modifications[ name ];
+							const modifications = has( this.modifications, name ) && this.modifications[ name ];
 
-								if ( modifications ) {
-									return sequence( modifications, include );
-								}
-							})
-							.then( () => {
-								return nodes;
-							});
-					};
+							if ( modifications ) {
+								return sequence( modifications, include );
+							}
+						})
+						.then( () => {
+							return nodes;
+						});
+				};
 
-					promise = include( statement );
-				}
+				promise = include( statement );
 			}
-
-			this.definitionPromises[ name ] = promise || emptyArrayPromise;
 		}
 
+
+
+		this.definitionPromises[ name ] = promise || emptyArrayPromise;
 		return this.definitionPromises[ name ];
 	}
 
 	rename ( name, replacement ) {
-		if ( hasOwnProp.call( this.nameReplacements, name ) ) {
-			throw new Error( 'Cannot rename an identifier twice' );
-		}
+		this.canonicalNames[ name ] = replacement;
+	}
 
-		this.nameReplacements[ name ] = replacement;
+	suggestDefaultName ( name ) {
+		if ( !this.defaultExportName ) {
+			this.defaultExportName = name;
+		}
 	}
+
+	// rename ( name, replacement, force ) {
+	// 	if ( has( this.nameReplacements, name ) ) {
+	// 		throw new Error( 'Cannot rename an identifier twice' );
+	// 	}
+
+	// 	if ( !force ) {
+	// 		replacement = this.bundle.getSafeReplacement( replacement, this );
+	// 	}
+
+	// 	if ( name === replacement ) {
+	// 		return;
+	// 	}
+
+	// 	console.log( 'renamining %s : %s -> %s (%s)', this.relativePath, name, replacement, force );
+
+	// 	const index = this.definedNames.indexOf( name );
+	// 	if ( ~index ) {
+	// 		this.definedNames[ index ] = replacement;
+	// 	}
+
+	// 	this.nameReplacements[ name ] = replacement;
+	// }
 }

src/finalisers/umd.js

@@ -8,7 +8,7 @@ export default function umd ( bundle, magicString, options ) {
 			factory((global.${options.globalName} = {}));
 		}(this, function (exports) { 'use strict';
 
-		`.replace( /^\t\t/gm, '' ).replace( /^\t/g, indentStr );
+		`.replace( /^\t\t/gm, '' ).replace( /^\t/gm, indentStr );
 
 	const exports = bundle.entryModule.exports;
 

src/utils/object.js

@@ -1 +1,7 @@
-export const hasOwnProp = Object.prototype.hasOwnProperty;
+export const keys = Object.keys;
+
+export const hasOwnProp = Object.prototype.hasOwnProperty;
+
+export function has ( obj, prop ) {
+	return hasOwnProp.call( obj, prop );
+}

src/utils/replaceIdentifiers.js

@@ -1,5 +1,5 @@
 import walk from '../ast/walk';
-import { hasOwnProp } from './object';
+import { has } from './object';
 
 export default function replaceIdentifiers ( statement, snippet, names ) {
 	const replacementStack = [ names ];
@@ -33,11 +33,7 @@ export default function replaceIdentifiers ( statement, snippet, names ) {
 			}
 
 			if ( node.type === 'Identifier' && parent.type !== 'MemberExpression' ) {
-				let name = node.name;
-
-				while ( hasOwnProp.call( names, name ) && name !== names[ name ] ) {
-					name = names[ name ];
-				}
+				const name = has( names, node.name ) && names[ node.name ];
 
 				if ( name && name !== node.name ) {
 					snippet.overwrite( node.start, node.end, name );

src/utils/sanitize.js

@@ -0,0 +1,6 @@
+export default function sanitize ( name ) {
+	name = name.replace( /[^$_0-9a-zA-Z]/g, '_' );
+	if ( !/[$_a-zA-Z]/.test( name ) ) name = `_${name}`;
+
+	return name;
+}

test/samples/consistent-renaming-b/_config.js

@@ -0,0 +1,3 @@
+module.exports = {
+	description: 'consistent renaming test b'
+};

test/samples/consistent-renaming-b/altdir/two.js

@@ -0,0 +1,5 @@
+function two () {
+	return 2;
+}
+
+export { two };

test/samples/consistent-renaming-b/main.js

@@ -0,0 +1,5 @@
+import one from './subdir/one';
+import Two from './subdir/two';
+
+assert.equal( one(), 1 );
+assert.equal( Two(), 2 );

test/samples/consistent-renaming-b/subdir/one.js

@@ -0,0 +1,5 @@
+import { two } from '../altdir/two';
+
+export default function one () {
+  return two() - 1;
+}

test/samples/consistent-renaming-b/subdir/two.js

@@ -0,0 +1,5 @@
+import { two as _two } from '../altdir/two';
+
+export default function two () {
+  return _two();
+}

test/samples/consistent-renaming-c/-internal.js

@@ -0,0 +1,6 @@
+/*** -internal.js */
+function two () {
+	return 99;
+}
+
+export { two };

test/samples/consistent-renaming-c/_config.js

@@ -0,0 +1,3 @@
+module.exports = {
+	description: 'consistent renaming test c'
+};

test/samples/consistent-renaming-c/main.js

@@ -0,0 +1,6 @@
+import One from './one';
+import two from './two';
+
+assert.equal( One(), 1 );
+assert.equal( two(), 2 );
+assert.equal( One.two(), 99 );

test/samples/consistent-renaming-c/one.js

@@ -0,0 +1,11 @@
+import three from './one/three';
+import Two from './one/two';
+
+export default function One () {
+	return 1;
+}
+
+One.three = three;
+
+/*** one.js */
+One.two = Two;

test/samples/consistent-renaming-c/one/three.js

@@ -0,0 +1,5 @@
+import { two } from '../-internal';
+
+export default function three () {
+  return 1 + two();
+}

test/samples/consistent-renaming-c/one/two.js

@@ -0,0 +1,6 @@
+import { two as _two } from '../-internal';
+
+/*** one/two.js */
+export default function two () {
+  return _two();
+}

test/samples/consistent-renaming-c/two.js

@@ -0,0 +1,6 @@
+import One from './one';
+
+/*** two.js */
+export default function two () {
+	return 2;
+}

test/samples/consistent-renaming-d/Baz.js

@@ -0,0 +1,5 @@
+function Baz () {
+	this.isBaz = true;
+}
+
+export default Baz;

test/samples/consistent-renaming-d/_config.js

@@ -0,0 +1,4 @@
+module.exports = {
+	description: 'consistent renaming test d',
+	// solo: true
+};

test/samples/consistent-renaming-d/bar.js

@@ -0,0 +1,12 @@
+import Baz from './Baz';
+import Foo from './foo';
+
+function Bar () {
+	this.inheritsFromBaz = this.isBaz;
+}
+
+Bar.prototype = new Baz();
+
+export default function bar() {
+  return new Bar();
+}

test/samples/consistent-renaming-d/foo.js

@@ -0,0 +1,5 @@
+import baz from './foo/baz';
+
+export default function Foo () {}
+
+Foo.baz = baz;

test/samples/consistent-renaming-d/foo/baz.js

@@ -0,0 +1,5 @@
+import Baz from '../Baz';
+
+export default function baz () {
+  return new Baz();
+}

test/samples/consistent-renaming-d/main.js

@@ -0,0 +1,6 @@
+import Foo from './foo';
+import bar from './bar';
+
+const baz = Foo.baz();
+assert.ok( baz.isBaz );
+assert.ok( bar().inheritsFromBaz );

test/samples/consistent-renaming-d/rsvp/all-settled.js

@@ -0,0 +1,11 @@
+import Enumerator from './enumerator';
+import Promise from './promise';
+
+function AllSettled () {}
+
+AllSettled.prototype = o_create(Enumerator.prototype);
+AllSettled.prototype._superConstructor = Enumerator;
+
+export default function allSettled(entries, label) {
+  return new AllSettled();
+}

test/samples/consistent-renaming-d/rsvp/enumerator.js

@@ -0,0 +1,3 @@
+function Enumerator () {}
+
+export default Enumerator;

test/samples/consistent-renaming-d/rsvp/promise.js

@@ -0,0 +1,5 @@
+import all from './promise/all';
+
+export default function Promise () {}
+
+Promise.all = all;

test/samples/consistent-renaming-d/rsvp/promise/all.js

@@ -0,0 +1,5 @@
+import Enumerator from '../enumerator';
+
+export default function all () {
+  return new Enumerator();
+}

test/samples/consistent-renaming-e/_config.js

@@ -0,0 +1,3 @@
+module.exports = {
+	description: 'consistent renaming test e'
+}

test/samples/consistent-renaming-e/a.js

@@ -0,0 +1,7 @@
+import { b } from './utils';
+
+function c () { console.log( 'main/c' ); }
+
+export default function a () {
+	return 'EH? ' + b();
+}

test/samples/consistent-renaming-e/b.js

@@ -0,0 +1,5 @@
+function c () { console.log( 'a/c' ); }
+
+export default function b () {
+	return 42;
+}

test/samples/consistent-renaming-e/main.js

@@ -0,0 +1,5 @@
+import a from './a';
+import b from './b';
+
+assert.equal( a(), 'EH? BEE' );
+assert.equal( b(), 42 );

test/samples/consistent-renaming-e/utils.js

@@ -0,0 +1,3 @@
+export function b () {
+	return 'BEE';
+}

test/samples/consistent-renaming/_config.js

@@ -1,3 +1,3 @@
 module.exports = {
-	description: 'renames identifiers consistently'
+	description: 'consistent renaming test'
 };

test/test.js

@@ -1,4 +1,5 @@
 require( 'source-map-support' ).install();
+require( 'console-group' ).install();
 
 var path = require( 'path' );
 var sander = require( 'sander' );
@@ -17,6 +18,8 @@ describe( 'rollup', function () {
 	});
 
 	sander.readdirSync( SAMPLES ).forEach( function ( dir ) {
+		if ( dir[0] === '.' ) return; // .DS_Store...
+
 		var config = require( SAMPLES + '/' + dir + '/_config' );
 
 		( config.solo ? it.only : it )( config.description, function () {