Merge pull request #777 from rollup/gh-763

use resolved IDs for external modules to allow options.globals to work with relative imports

browser/path.js

@@ -0,0 +1,80 @@
+export const absolutePath = /^(?:\/|(?:[A-Za-z]:)?[\\|\/])/;
+export const relativePath = /^\.?\.\//;
+
+export function isAbsolute ( path ) {
+	return absolutePath.test( path );
+}
+
+export function isRelative ( path ) {
+	return relativePath.test( path );
+}
+
+export function normalize ( path ) {
+	return path.replace( /\\/g, '/' );
+}
+
+export function basename ( path ) {
+	return path.split( /(\/|\\)/ ).pop();
+}
+
+export function dirname ( path ) {
+	const match = /(\/|\\)[^\/\\]*$/.exec( path );
+	if ( !match ) return '.';
+
+	const dir = path.slice( 0, -match[0].length );
+
+	// If `dir` is the empty string, we're at root.
+	return dir ? dir : '/';
+}
+
+export function extname ( path ) {
+	const match = /\.[^\.]+$/.exec( basename( path ) );
+	if ( !match ) return '';
+	return match[0];
+}
+
+export function relative ( from, to ) {
+	const fromParts = from.split( /[\/\\]/ ).filter( Boolean );
+	const toParts = to.split( /[\/\\]/ ).filter( Boolean );
+
+	while ( fromParts[0] && toParts[0] && fromParts[0] === toParts[0] ) {
+		fromParts.shift();
+		toParts.shift();
+	}
+
+	while ( toParts[0] === '.' || toParts[0] === '..' ) {
+		const toPart = toParts.shift();
+		if ( toPart === '..' ) {
+			fromParts.pop();
+		}
+	}
+
+	while ( fromParts.pop() ) {
+		toParts.unshift( '..' );
+	}
+
+	return toParts.join( '/' );
+}
+
+export function resolve ( ...paths ) {
+	let resolvedParts = paths.shift().split( /[\/\\]/ );
+
+	paths.forEach( path => {
+		if ( isAbsolute( path ) ) {
+			resolvedParts = path.split( /[\/\\]/ );
+		} else {
+			const parts = path.split( /[\/\\]/ );
+
+			while ( parts[0] === '.' || parts[0] === '..' ) {
+				const part = parts.shift();
+				if ( part === '..' ) {
+					resolvedParts.pop();
+				}
+			}
+
+			resolvedParts.push.apply( resolvedParts, parts );
+		}
+	});
+
+	return resolvedParts.join( '/' ); // TODO windows...
+}

rollup.config.browser.js

@@ -3,7 +3,8 @@ import config from './rollup.config.js';
 
 config.plugins.push({
 	load: function ( id ) {
-		if ( ~id.indexOf( 'fs.js' ) ) return readFileSync( 'browser/fs.js' ).toString();
+		if ( ~id.indexOf( 'fs.js' ) ) return readFileSync( 'browser/fs.js', 'utf-8' );
+		if ( ~id.indexOf( 'path.js' ) ) return readFileSync( 'browser/path.js', 'utf-8' );
 	}
 });
 

src/Bundle.js

@@ -9,14 +9,13 @@ import ensureArray from './utils/ensureArray.js';
 import { load, makeOnwarn, resolveId } from './utils/defaults.js';
 import getExportMode from './utils/getExportMode.js';
 import getIndentString from './utils/getIndentString.js';
-import { unixizePath } from './utils/normalizePlatform.js';
 import { mapSequence } from './utils/promise.js';
 import transform from './utils/transform.js';
 import transformBundle from './utils/transformBundle.js';
 import collapseSourcemaps from './utils/collapseSourcemaps.js';
 import SOURCEMAPPING_URL from './utils/sourceMappingURL.js';
 import callIfFunction from './utils/callIfFunction.js';
-import { dirname, isRelative, isAbsolute, relative, resolve } from './utils/path.js';
+import { dirname, isRelative, isAbsolute, normalize, relative, resolve } from './utils/path.js';
 
 export default class Bundle {
 	constructor ( options ) {
@@ -35,7 +34,7 @@ export default class Bundle {
 			}
 		});
 
-		this.entry = unixizePath( options.entry );
+		this.entry = normalize( options.entry );
 		this.entryId = null;
 		this.entryModule = null;
 
@@ -64,7 +63,7 @@ export default class Bundle {
 		if ( typeof options.external === 'function' ) {
 			this.isExternal = options.external;
 		} else {
-			const ids = ensureArray( options.external ).map( id => id.replace( /[\/\\]/g, '/' ) );
+			const ids = ensureArray( options.external );
 			this.isExternal = id => ids.indexOf( id ) !== -1;
 		}
 
@@ -227,40 +226,28 @@ export default class Bundle {
 		return mapSequence( module.sources, source => {
 			return this.resolveId( source, module.id )
 				.then( resolvedId => {
-					let externalName;
+					const externalId = resolvedId || (
-					if ( resolvedId ) {
+						isRelative( source ) ? resolve( module.id, '..', source ) : source
-						// If the `resolvedId` is supposed to be external, make it so.
+					);
-						externalName = resolvedId.replace( /[\/\\]/g, '/' );
+
-					} else if ( isRelative( source ) ) {
+					let isExternal = this.isExternal( externalId );
-						// This could be an external, relative dependency, based on the current module's parent dir.
+
-						externalName = resolve( module.id, '..', source );
+					if ( !resolvedId && !isExternal ) {
+						if ( isRelative( source ) ) throw new Error( `Could not resolve ${source} from ${module.id}` );
+
+						this.onwarn( `Treating '${source}' as external dependency` );
+						isExternal = true;
 					}
-					const forcedExternal = externalName && this.isExternal( externalName );
-
-					if ( !resolvedId || forcedExternal ) {
-						let normalizedExternal = source;
-
-						if ( !forcedExternal ) {
-							if ( isRelative( source ) ) throw new Error( `Could not resolve ${source} from ${module.id}` );
-							if ( !this.isExternal( source ) ) this.onwarn( `Treating '${source}' as external dependency` );
-						} else if ( resolvedId ) {
-							if ( isRelative(resolvedId) || isAbsolute(resolvedId) ) {
-								// Try to deduce relative path from entry dir if resolvedId is defined as a relative path.
-								normalizedExternal = this.getPathRelativeToEntryDirname( resolvedId );
-							} else {
-								normalizedExternal = resolvedId;
-							}
-						}
-						module.resolvedIds[ source ] = normalizedExternal;
 
-						if ( !this.moduleById.has( normalizedExternal ) ) {
+					if ( isExternal ) {
-							const module = new ExternalModule( normalizedExternal );
+						module.resolvedIds[ source ] = externalId;
+
+						if ( !this.moduleById.has( externalId ) ) {
+							const module = new ExternalModule( externalId, this.getPathRelativeToEntryDirname( externalId ) );
 							this.externalModules.push( module );
-							this.moduleById.set( normalizedExternal, module );
+							this.moduleById.set( externalId, module );
 						}
-					}
+					} else {
-
-					else {
 						if ( resolvedId === module.id ) {
 							throw new Error( `A module cannot import itself (${resolvedId})` );
 						}
@@ -273,16 +260,14 @@ export default class Bundle {
 	}
 
 	getPathRelativeToEntryDirname ( resolvedId ) {
-		// Get a path relative to the resolved entry directory
+		if ( isRelative( resolvedId ) || isAbsolute( resolvedId ) ) {
-		const entryDirname = dirname( this.entryId );
+			const entryDirname = dirname( this.entryId );
-		const relativeToEntry = relative( entryDirname, resolvedId );
+			const relativeToEntry = normalize( relative( entryDirname, resolvedId ) );
 
-		if ( isRelative( relativeToEntry )) {
+			return isRelative( relativeToEntry ) ? relativeToEntry : `./${relativeToEntry}`;
-			return relativeToEntry;
 		}
 
-		// The path is missing the `./` prefix
+		return resolvedId;
-		return `./${relativeToEntry}`;
 	}
 
 	render ( options = {} ) {
@@ -357,7 +342,7 @@ export default class Bundle {
 				map = magicString.generateMap({ file, includeContent: true });
 			}
 
-			map.sources = map.sources.map( unixizePath );
+			map.sources = map.sources.map( normalize );
 		}
 
 		return { code, map };

src/ExternalModule.js

@@ -3,9 +3,11 @@ import makeLegalIdentifier from './utils/makeLegalIdentifier.js';
 import { ExternalDeclaration } from './Declaration.js';
 
 export default class ExternalModule {
-	constructor ( id ) {
+	constructor ( id, relativePath ) {
 		this.id = id;
-		this.name = makeLegalIdentifier( id );
+		this.path = relativePath;
+
+		this.name = makeLegalIdentifier( relativePath );
 
 		this.nameSuggestions = blank();
 		this.mostCommonSuggestion = 0;

src/finalisers/amd.js

@@ -1,10 +1,10 @@
-import { getName, quoteId } from '../utils/map-helpers.js';
+import { getName, quotePath } from '../utils/map-helpers.js';
 import getInteropBlock from './shared/getInteropBlock.js';
 import getExportBlock from './shared/getExportBlock.js';
 import esModuleExport from './shared/esModuleExport.js';
 
 export default function amd ( bundle, magicString, { exportMode, indentString }, options ) {
-	let deps = bundle.externalModules.map( quoteId );
+	let deps = bundle.externalModules.map( quotePath );
 	let args = bundle.externalModules.map( getName );
 
 	if ( exportMode === 'named' ) {

src/finalisers/cjs.js

@@ -14,20 +14,20 @@ export default function cjs ( bundle, magicString, { exportMode }, options ) {
 		.map( module => {
 			if ( module.declarations.default ) {
 				if ( module.exportsNamespace ) {
-					return `${varOrConst} ${module.name} = require('${module.id}');` +
+					return `${varOrConst} ${module.name} = require('${module.path}');` +
 						`\n${varOrConst} ${module.name}__default = ${module.name}['default'];`;
 				}
 
 				needsInterop = true;
 
 				if ( module.exportsNames ) {
-					return `${varOrConst} ${module.name} = require('${module.id}');` +
+					return `${varOrConst} ${module.name} = require('${module.path}');` +
 						`\n${varOrConst} ${module.name}__default = _interopDefault(${module.name});`;
 				}
 
-				return `${varOrConst} ${module.name} = _interopDefault(require('${module.id}'));`;
+				return `${varOrConst} ${module.name} = _interopDefault(require('${module.path}'));`;
 			} else {
-				return `${varOrConst} ${module.name} = require('${module.id}');`;
+				return `${varOrConst} ${module.name} = require('${module.path}');`;
 			}
 		})
 		.join( '\n' );

src/finalisers/es.js

@@ -42,8 +42,8 @@ export default function es ( bundle, magicString ) {
 			return specifiersList
 				.map( specifiers =>
 					specifiers.length ?
-						`import ${specifiers.join( ', ' )} from '${module.id}';` :
+						`import ${specifiers.join( ', ' )} from '${module.path}';` :
-						`import '${module.id}';`
+						`import '${module.path}';`
 				)
 				.join( '\n' );
 		})

src/finalisers/umd.js

@@ -1,5 +1,5 @@
 import { blank } from '../utils/object.js';
-import { getName, quoteId, req } from '../utils/map-helpers.js';
+import { getName, quotePath, req } from '../utils/map-helpers.js';
 import getInteropBlock from './shared/getInteropBlock.js';
 import getExportBlock from './shared/getExportBlock.js';
 import getGlobalNameMaker from './shared/getGlobalNameMaker.js';
@@ -23,7 +23,7 @@ export default function umd ( bundle, magicString, { exportMode, indentString },
 
 	const globalNameMaker = getGlobalNameMaker( options.globals || blank(), bundle.onwarn );
 
-	let amdDeps = bundle.externalModules.map( quoteId );
+	let amdDeps = bundle.externalModules.map( quotePath );
 	let cjsDeps = bundle.externalModules.map( req );
 	let globalDeps = bundle.externalModules.map( module => `global.${globalNameMaker( module )}` );
 

src/utils/map-helpers.js

@@ -2,10 +2,10 @@ export function getName ( x ) {
 	return x.name;
 }
 
-export function quoteId ( x ) {
+export function quotePath ( x ) {
-	return `'${x.id}'`;
+	return `'${x.path}'`;
 }
 
 export function req ( x ) {
-	return `require('${x.id}')`;
+	return `require('${x.path}')`;
 }

src/utils/normalizePlatform.js

@@ -1,3 +0,0 @@
-export function unixizePath ( path ) {
-	return path.split( /[\/\\]/ ).join( '/' );
-}

src/utils/path.js

@@ -1,5 +1,3 @@
-// TODO does this all work on windows?
-
 export const absolutePath = /^(?:\/|(?:[A-Za-z]:)?[\\|\/])/;
 export const relativePath = /^\.?\.\//;
 
@@ -11,68 +9,8 @@ export function isRelative ( path ) {
 	return relativePath.test( path );
 }
 
-export function basename ( path ) {
+export function normalize ( path ) {
-	return path.split( /(\/|\\)/ ).pop();
+	return path.replace( /\\/g, '/' );
-}
-
-export function dirname ( path ) {
-	const match = /(\/|\\)[^\/\\]*$/.exec( path );
-	if ( !match ) return '.';
-
-	const dir = path.slice( 0, -match[0].length );
-
-	// If `dir` is the empty string, we're at root.
-	return dir ? dir : '/';
-}
-
-export function extname ( path ) {
-	const match = /\.[^\.]+$/.exec( basename( path ) );
-	if ( !match ) return '';
-	return match[0];
-}
-
-export function relative ( from, to ) {
-	const fromParts = from.split( /[\/\\]/ ).filter( Boolean );
-	const toParts = to.split( /[\/\\]/ ).filter( Boolean );
-
-	while ( fromParts[0] && toParts[0] && fromParts[0] === toParts[0] ) {
-		fromParts.shift();
-		toParts.shift();
-	}
-
-	while ( toParts[0] === '.' || toParts[0] === '..' ) {
-		const toPart = toParts.shift();
-		if ( toPart === '..' ) {
-			fromParts.pop();
-		}
-	}
-
-	while ( fromParts.pop() ) {
-		toParts.unshift( '..' );
-	}
-
-	return toParts.join( '/' );
 }
 
-export function resolve ( ...paths ) {
+export * from 'path';
-	let resolvedParts = paths.shift().split( /[\/\\]/ );
-
-	paths.forEach( path => {
-		if ( isAbsolute( path ) ) {
-			resolvedParts = path.split( /[\/\\]/ );
-		} else {
-			const parts = path.split( /[\/\\]/ );
-
-			while ( parts[0] === '.' || parts[0] === '..' ) {
-				const part = parts.shift();
-				if ( part === '..' ) {
-					resolvedParts.pop();
-				}
-			}
-
-			resolvedParts.push.apply( resolvedParts, parts );
-		}
-	});
-
-	return resolvedParts.join( '/' ); // TODO windows...
-}

test/cli/config-external-function/rollup.config.js

@@ -1,18 +1,14 @@
 import assert from 'assert';
-import { resolve, sep } from 'path';
+import { resolve } from 'path';
 
-var config = resolve( './_config.js' ).split(sep).join('/');
+var config = resolve( './_config.js' );
 
 export default {
 	entry: 'main.js',
 	format: 'cjs',
 
-	external: function (id) {
+	external: function ( id ) {
-		if (id === config) {
+		return id === config;
-			return true;
-		}
-
-		return false;
 	},
 
 	plugins: [

test/form/relative-external-with-global/_config.js

@@ -0,0 +1,13 @@
+const { resolve } = require( 'path' );
+
+const throttle = resolve( __dirname, 'lib/throttle.js' );
+
+module.exports = {
+	description: 'applies globals to externalised relative imports',
+	options: {
+		external: [ throttle ],
+		globals: {
+			[ throttle ]: 'Lib.throttle'
+		}
+	}
+};

test/form/relative-external-with-global/_expected/amd.js

@@ -0,0 +1,11 @@
+define(['./lib/throttle.js'], function (throttle) { 'use strict';
+
+	throttle = 'default' in throttle ? throttle['default'] : throttle;
+
+	const fn = throttle( () => {
+		console.log( '.' );
+	}, 500 );
+
+	window.addEventListener( 'mousemove', throttle );
+
+});

test/form/relative-external-with-global/_expected/cjs.js

@@ -0,0 +1,11 @@
+'use strict';
+
+function _interopDefault (ex) { return (ex && (typeof ex === 'object') && 'default' in ex) ? ex['default'] : ex; }
+
+var throttle = _interopDefault(require('./lib/throttle.js'));
+
+const fn = throttle( () => {
+	console.log( '.' );
+}, 500 );
+
+window.addEventListener( 'mousemove', throttle );

test/form/relative-external-with-global/_expected/es.js

@@ -0,0 +1,7 @@
+import throttle from './lib/throttle.js';
+
+const fn = throttle( () => {
+	console.log( '.' );
+}, 500 );
+
+window.addEventListener( 'mousemove', throttle );

test/form/relative-external-with-global/_expected/iife.js

@@ -0,0 +1,12 @@
+(function (throttle) {
+	'use strict';
+
+	throttle = 'default' in throttle ? throttle['default'] : throttle;
+
+	const fn = throttle( () => {
+		console.log( '.' );
+	}, 500 );
+
+	window.addEventListener( 'mousemove', throttle );
+
+}(Lib.throttle));

test/form/relative-external-with-global/_expected/umd.js

@@ -0,0 +1,15 @@
+(function (global, factory) {
+	typeof exports === 'object' && typeof module !== 'undefined' ? factory(require('./lib/throttle.js')) :
+	typeof define === 'function' && define.amd ? define(['./lib/throttle.js'], factory) :
+	(factory(global.Lib.throttle));
+}(this, function (throttle) { 'use strict';
+
+	throttle = 'default' in throttle ? throttle['default'] : throttle;
+
+	const fn = throttle( () => {
+		console.log( '.' );
+	}, 500 );
+
+	window.addEventListener( 'mousemove', throttle );
+
+}));

test/form/relative-external-with-global/main.js

@@ -0,0 +1,7 @@
+import throttle from './lib/throttle.js';
+
+const fn = throttle( () => {
+	console.log( '.' );
+}, 500 );
+
+window.addEventListener( 'mousemove', throttle );

test/function/double-named-export-from/_config.js

@@ -1,15 +1,13 @@
-const path = require('path');
+const { resolve } = require('path');
 const assert = require( 'assert' );
 
-function normalize ( file ) {
+const r = path => resolve( __dirname, path );
-	return path.resolve( __dirname, file ).split( '\\' ).join( '/' );
-}
 
 module.exports = {
 	description: 'throws on duplicate export * from',
 	warnings ( warnings ) {
 		assert.deepEqual( warnings, [
-			`Conflicting namespaces: ${normalize('main.js')} re-exports 'foo' from both ${normalize('foo.js')} (will be ignored) and ${normalize('deep.js')}.`
+			`Conflicting namespaces: ${r('main.js')} re-exports 'foo' from both ${r('foo.js')} (will be ignored) and ${r('deep.js')}.`
 		]);
 	}
 };

test/test.js

@@ -40,6 +40,8 @@ function loadConfig ( path ) {
 	try {
 		return require( path );
 	} catch ( err ) {
+		console.error( err.message );
+		console.error( err.stack );
 		throw new Error( 'Failed to load ' + path + '. An old test perhaps? You should probably delete the directory' );
 	}
 }
@@ -552,7 +554,7 @@ describe( 'rollup', function () {
 	describe( 'hooks', () => {
 		it( 'passes bundle & output object to ongenerate & onwrite hooks', () => {
 			var dest = path.join( __dirname, 'tmp/bundle.js' );
-			
+
 			return rollup.rollup({
 				entry: 'entry',
 				plugins: [