From d5c232d6ccc1f5f75b246e84323d4aefaabe8cb8 Mon Sep 17 00:00:00 2001 From: Bogdan Chadkin Date: Sun, 19 Jun 2016 23:06:46 +0300 Subject: [PATCH 1/6] Fail on duplicating in export * from --- src/Bundle.js | 19 ++++++++++++++++++- src/utils/object.js | 3 ++- .../double-named-export-from/_config.js | 14 ++++++++++++++ test/function/double-named-export-from/bar.js | 1 + .../function/double-named-export-from/deep.js | 1 + test/function/double-named-export-from/foo.js | 1 + .../function/double-named-export-from/main.js | 2 ++ 7 files changed, 39 insertions(+), 2 deletions(-) create mode 100644 test/function/double-named-export-from/_config.js create mode 100644 test/function/double-named-export-from/bar.js create mode 100644 test/function/double-named-export-from/deep.js create mode 100644 test/function/double-named-export-from/foo.js create mode 100644 test/function/double-named-export-from/main.js diff --git a/src/Bundle.js b/src/Bundle.js index 7d6904d..4c2f5a8 100644 --- a/src/Bundle.js +++ b/src/Bundle.js @@ -210,7 +210,24 @@ export default class Bundle { this.modules.push( module ); this.moduleById.set( id, module ); - return this.fetchAllDependencies( module ).then( () => module ); + return this.fetchAllDependencies( module ).then( () => { + module.exportsAll = blank(); + keys( module.exports ).forEach( name => { + module.exportsAll[name] = module.id; + }); + module.exportAllSources.forEach( source => { + const id = module.resolvedIds[ source ]; + const exportAllModule = this.moduleById.get( id ); + keys( exportAllModule.exportsAll ).forEach( name => { + if ( name in module.exportsAll ) { + throw new Error( `A module cannot have multiple exports with the same name ('${name}')` + + ` from ${module.exportsAll[ name ] } and ${exportAllModule.exportsAll[ name ]}` ); + } + module.exportsAll[ name ] = exportAllModule.exportsAll[ name ]; + }); + }); + return module; + }); }); } diff --git a/src/utils/object.js b/src/utils/object.js index 688b1ca..62c3a07 100644 --- a/src/utils/object.js +++ b/src/utils/object.js @@ -1,3 +1,4 @@ +const { hasOwnProperty } = Object.prototype; export const { keys } = Object; export function blank () { @@ -11,7 +12,7 @@ export function forOwn ( object, func ) { export function assign ( target, ...sources ) { sources.forEach( source => { for ( let key in source ) { - if ( source.hasOwnProperty( key ) ) target[ key ] = source[ key ]; + if ( hasOwnProperty.call( source, key ) ) target[ key ] = source[ key ]; } }); diff --git a/test/function/double-named-export-from/_config.js b/test/function/double-named-export-from/_config.js new file mode 100644 index 0000000..1b402d3 --- /dev/null +++ b/test/function/double-named-export-from/_config.js @@ -0,0 +1,14 @@ +const path = require('path'); +const assert = require( 'assert' ); + +function normalize( file ) { + return path.resolve( __dirname, file ).split( '\\' ).join( '/' ); +} + +module.exports = { + description: 'throws on duplicate export * from', + error: err => { + assert.equal( err.message, `A module cannot have multiple exports with the same name ('foo')` + + ` from ${normalize( 'foo.js' )} and ${normalize( 'deep.js' )}` ); + } +}; diff --git a/test/function/double-named-export-from/bar.js b/test/function/double-named-export-from/bar.js new file mode 100644 index 0000000..0dfd7e9 --- /dev/null +++ b/test/function/double-named-export-from/bar.js @@ -0,0 +1 @@ +export * from './deep.js'; \ No newline at end of file diff --git a/test/function/double-named-export-from/deep.js b/test/function/double-named-export-from/deep.js new file mode 100644 index 0000000..a7b877b --- /dev/null +++ b/test/function/double-named-export-from/deep.js @@ -0,0 +1 @@ +export var foo = 2; \ No newline at end of file diff --git a/test/function/double-named-export-from/foo.js b/test/function/double-named-export-from/foo.js new file mode 100644 index 0000000..467f528 --- /dev/null +++ b/test/function/double-named-export-from/foo.js @@ -0,0 +1 @@ +export var foo = 1; \ No newline at end of file diff --git a/test/function/double-named-export-from/main.js b/test/function/double-named-export-from/main.js new file mode 100644 index 0000000..ae6aade --- /dev/null +++ b/test/function/double-named-export-from/main.js @@ -0,0 +1,2 @@ +export * from './foo.js'; +export * from './bar.js'; \ No newline at end of file From 705b1e97a5d24de68be26d390b3d6e9f0be29d61 Mon Sep 17 00:00:00 2001 From: Bogdan Chadkin Date: Sun, 19 Jun 2016 23:09:47 +0300 Subject: [PATCH 2/6] Revert assign mod --- src/utils/object.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/utils/object.js b/src/utils/object.js index 62c3a07..688b1ca 100644 --- a/src/utils/object.js +++ b/src/utils/object.js @@ -1,4 +1,3 @@ -const { hasOwnProperty } = Object.prototype; export const { keys } = Object; export function blank () { @@ -12,7 +11,7 @@ export function forOwn ( object, func ) { export function assign ( target, ...sources ) { sources.forEach( source => { for ( let key in source ) { - if ( hasOwnProperty.call( source, key ) ) target[ key ] = source[ key ]; + if ( source.hasOwnProperty( key ) ) target[ key ] = source[ key ]; } }); From 69176879961a937ae4547d1c80b56975105e74be Mon Sep 17 00:00:00 2001 From: Bogdan Chadkin Date: Mon, 20 Jun 2016 07:34:47 +0300 Subject: [PATCH 3/6] Fail on export all dup optionally --- src/Bundle.js | 5 +++++ src/rollup.js | 3 ++- test/function/double-named-export-from/_config.js | 3 +++ test/test.js | 2 +- 4 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/Bundle.js b/src/Bundle.js index 4c2f5a8..1f7c78a 100644 --- a/src/Bundle.js +++ b/src/Bundle.js @@ -26,6 +26,8 @@ export default class Bundle { }); } + this.failOnExportAllDup = Boolean( options.failOnExportAllDup ); + this.plugins = ensureArray( options.plugins ); this.plugins.forEach( plugin => { @@ -211,6 +213,9 @@ export default class Bundle { this.moduleById.set( id, module ); return this.fetchAllDependencies( module ).then( () => { + if ( !this.failOnExportAllDup ) { + return module; + } module.exportsAll = blank(); keys( module.exports ).forEach( name => { module.exportsAll[name] = module.id; diff --git a/src/rollup.js b/src/rollup.js index a9c50fd..5399dc3 100644 --- a/src/rollup.js +++ b/src/rollup.js @@ -31,7 +31,8 @@ const ALLOWED_KEYS = [ 'sourceMapFile', 'targets', 'treeshake', - 'useStrict' + 'useStrict', + 'failOnExportAllDup' ]; export function rollup ( options ) { diff --git a/test/function/double-named-export-from/_config.js b/test/function/double-named-export-from/_config.js index 1b402d3..0aaa1f5 100644 --- a/test/function/double-named-export-from/_config.js +++ b/test/function/double-named-export-from/_config.js @@ -7,6 +7,9 @@ function normalize( file ) { module.exports = { description: 'throws on duplicate export * from', + options: { + failOnExportAllDup: true + }, error: err => { assert.equal( err.message, `A module cannot have multiple exports with the same name ('foo')` + ` from ${normalize( 'foo.js' )} and ${normalize( 'deep.js' )}` ); diff --git a/test/test.js b/test/test.js index 30ec5f2..6bd60b1 100644 --- a/test/test.js +++ b/test/test.js @@ -76,7 +76,7 @@ describe( 'rollup', function () { return rollup.rollup({ entry: 'x', plUgins: [] }).then( function () { throw new Error( 'Missing expected error' ); }, function ( err ) { - assert.equal( err.message, 'Unexpected key \'plUgins\' found, expected one of: acorn, banner, cache, dest, entry, exports, external, footer, format, globals, indent, intro, moduleId, moduleName, noConflict, onwarn, outro, plugins, preferConst, sourceMap, sourceMapFile, targets, treeshake, useStrict' ); + assert.equal( err.message, 'Unexpected key \'plUgins\' found, expected one of: acorn, banner, cache, dest, entry, exports, external, footer, format, globals, indent, intro, moduleId, moduleName, noConflict, onwarn, outro, plugins, preferConst, sourceMap, sourceMapFile, targets, treeshake, useStrict, failOnExportAllDup' ); }); }); }); From 87983313895ca2aea0ad0fb3c732636e96bc2e49 Mon Sep 17 00:00:00 2001 From: Bogdan Chadkin Date: Thu, 23 Jun 2016 09:02:32 +0300 Subject: [PATCH 4/6] Warn on namespace conflicts --- src/Bundle.js | 7 +------ src/rollup.js | 3 +-- test/function/double-named-export-from/_config.js | 9 ++++----- test/test.js | 2 +- 4 files changed, 7 insertions(+), 14 deletions(-) diff --git a/src/Bundle.js b/src/Bundle.js index 1f7c78a..988ef7e 100644 --- a/src/Bundle.js +++ b/src/Bundle.js @@ -26,8 +26,6 @@ export default class Bundle { }); } - this.failOnExportAllDup = Boolean( options.failOnExportAllDup ); - this.plugins = ensureArray( options.plugins ); this.plugins.forEach( plugin => { @@ -213,9 +211,6 @@ export default class Bundle { this.moduleById.set( id, module ); return this.fetchAllDependencies( module ).then( () => { - if ( !this.failOnExportAllDup ) { - return module; - } module.exportsAll = blank(); keys( module.exports ).forEach( name => { module.exportsAll[name] = module.id; @@ -225,7 +220,7 @@ export default class Bundle { const exportAllModule = this.moduleById.get( id ); keys( exportAllModule.exportsAll ).forEach( name => { if ( name in module.exportsAll ) { - throw new Error( `A module cannot have multiple exports with the same name ('${name}')` + + this.onwarn( `A module cannot have multiple exports with the same name ('${name}')` + ` from ${module.exportsAll[ name ] } and ${exportAllModule.exportsAll[ name ]}` ); } module.exportsAll[ name ] = exportAllModule.exportsAll[ name ]; diff --git a/src/rollup.js b/src/rollup.js index 5399dc3..a9c50fd 100644 --- a/src/rollup.js +++ b/src/rollup.js @@ -31,8 +31,7 @@ const ALLOWED_KEYS = [ 'sourceMapFile', 'targets', 'treeshake', - 'useStrict', - 'failOnExportAllDup' + 'useStrict' ]; export function rollup ( options ) { diff --git a/test/function/double-named-export-from/_config.js b/test/function/double-named-export-from/_config.js index 0aaa1f5..183bb4c 100644 --- a/test/function/double-named-export-from/_config.js +++ b/test/function/double-named-export-from/_config.js @@ -6,12 +6,11 @@ function normalize( file ) { } module.exports = { + solo: true, description: 'throws on duplicate export * from', - options: { - failOnExportAllDup: true - }, - error: err => { - assert.equal( err.message, `A module cannot have multiple exports with the same name ('foo')` + + warnings(warnings) { + assert.equal( warnings[0], `A module cannot have multiple exports with the same name ('foo')` + ` from ${normalize( 'foo.js' )} and ${normalize( 'deep.js' )}` ); + assert.equal( warnings.length, 1 ); } }; diff --git a/test/test.js b/test/test.js index 6bd60b1..30ec5f2 100644 --- a/test/test.js +++ b/test/test.js @@ -76,7 +76,7 @@ describe( 'rollup', function () { return rollup.rollup({ entry: 'x', plUgins: [] }).then( function () { throw new Error( 'Missing expected error' ); }, function ( err ) { - assert.equal( err.message, 'Unexpected key \'plUgins\' found, expected one of: acorn, banner, cache, dest, entry, exports, external, footer, format, globals, indent, intro, moduleId, moduleName, noConflict, onwarn, outro, plugins, preferConst, sourceMap, sourceMapFile, targets, treeshake, useStrict, failOnExportAllDup' ); + assert.equal( err.message, 'Unexpected key \'plUgins\' found, expected one of: acorn, banner, cache, dest, entry, exports, external, footer, format, globals, indent, intro, moduleId, moduleName, noConflict, onwarn, outro, plugins, preferConst, sourceMap, sourceMapFile, targets, treeshake, useStrict' ); }); }); }); From 5028da3a27b703a2ced9b02645ff7aea136b0aad Mon Sep 17 00:00:00 2001 From: Bogdan Chadkin Date: Thu, 23 Jun 2016 09:06:09 +0300 Subject: [PATCH 5/6] Test all --- test/function/double-named-export-from/_config.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/function/double-named-export-from/_config.js b/test/function/double-named-export-from/_config.js index 183bb4c..ee0880f 100644 --- a/test/function/double-named-export-from/_config.js +++ b/test/function/double-named-export-from/_config.js @@ -6,7 +6,6 @@ function normalize( file ) { } module.exports = { - solo: true, description: 'throws on duplicate export * from', warnings(warnings) { assert.equal( warnings[0], `A module cannot have multiple exports with the same name ('foo')` + From 84cccae433dc554af43dc3b0b4dfccbb2d2a8764 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 23 Jun 2016 14:28:16 -0400 Subject: [PATCH 6/6] change language to reflect the fact that conflicting namespaces are permitted, if discouraged --- src/Bundle.js | 3 +-- test/function/double-named-export-from/_config.js | 10 +++++----- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/Bundle.js b/src/Bundle.js index 988ef7e..489c45d 100644 --- a/src/Bundle.js +++ b/src/Bundle.js @@ -220,8 +220,7 @@ export default class Bundle { const exportAllModule = this.moduleById.get( id ); keys( exportAllModule.exportsAll ).forEach( name => { if ( name in module.exportsAll ) { - this.onwarn( `A module cannot have multiple exports with the same name ('${name}')` + - ` from ${module.exportsAll[ name ] } and ${exportAllModule.exportsAll[ name ]}` ); + this.onwarn( `Conflicting namespaces: ${module.id} re-exports '${name}' from both ${module.exportsAll[ name ]} (will be ignored) and ${exportAllModule.exportsAll[ name ]}.` ); } module.exportsAll[ name ] = exportAllModule.exportsAll[ name ]; }); diff --git a/test/function/double-named-export-from/_config.js b/test/function/double-named-export-from/_config.js index ee0880f..09cb582 100644 --- a/test/function/double-named-export-from/_config.js +++ b/test/function/double-named-export-from/_config.js @@ -1,15 +1,15 @@ const path = require('path'); const assert = require( 'assert' ); -function normalize( file ) { +function normalize ( file ) { return path.resolve( __dirname, file ).split( '\\' ).join( '/' ); } module.exports = { description: 'throws on duplicate export * from', - warnings(warnings) { - assert.equal( warnings[0], `A module cannot have multiple exports with the same name ('foo')` + - ` from ${normalize( 'foo.js' )} and ${normalize( 'deep.js' )}` ); - assert.equal( warnings.length, 1 ); + 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')}.` + ]); } };