From 6e839f2d38b8b0e058d990c283849af7ae237e27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Segersv=C3=A4rd?= Date: Sat, 26 Sep 2015 16:19:18 +0200 Subject: [PATCH] Extract namespace lookup optimisation into separate pass --- src/Bundle.js | 6 + src/Statement.js | 107 ---------------- src/optimise/namespace-lookup.js | 114 ++++++++++++++++++ .../_config.js | 5 + .../bar.js | 1 + .../foo.js | 1 + .../main.js | 6 + .../zoo.js | 7 ++ 8 files changed, 140 insertions(+), 107 deletions(-) create mode 100644 src/optimise/namespace-lookup.js create mode 100644 test/function/namespace-optimisation-before-exports/_config.js create mode 100644 test/function/namespace-optimisation-before-exports/bar.js create mode 100644 test/function/namespace-optimisation-before-exports/foo.js create mode 100644 test/function/namespace-optimisation-before-exports/main.js create mode 100644 test/function/namespace-optimisation-before-exports/zoo.js diff --git a/src/Bundle.js b/src/Bundle.js index 28347b1..96eeaa5 100644 --- a/src/Bundle.js +++ b/src/Bundle.js @@ -12,6 +12,8 @@ import getIndentString from './utils/getIndentString'; import { unixizePath } from './utils/normalizePlatform.js'; import Scope from './Scope'; +import optimiseNamespaceLookups from './optimise/namespace-lookup.js'; + export default class Bundle { constructor ( options ) { this.entry = options.entry; @@ -61,6 +63,10 @@ export default class Bundle { return Promise.resolve( this.resolveId( this.entry, undefined, this.resolveOptions ) ) .then( id => this.fetchModule( id ) ) .then( entryModule => { + this.modules.forEach( module => { + module.statements.forEach( optimiseNamespaceLookups ); + }); + this.entryModule = entryModule; this.exports = entryModule.exports; diff --git a/src/Statement.js b/src/Statement.js index 0170460..c8b8a8f 100644 --- a/src/Statement.js +++ b/src/Statement.js @@ -25,21 +25,6 @@ function isFunctionDeclaration ( node, parent ) { if ( node.type === 'FunctionExpression' && parent.type === 'VariableDeclarator' ) return true; } -// Extract the property access from a MemberExpression. -function property ( node ) { - return node.name ? `.${node.name}` : `[${node.value}]`; -} - -// Recursively traverse the chain of member expressions from `node`, -// returning the access, e.g. `foo.bar[17]` -function chainedMemberExpression ( node ) { - if ( node.object.type === 'MemberExpression' ) { - return chainedMemberExpression( node.object ) + property( node.property ); - } - - return node.object.name + property( node.property ); -} - export default class Statement { constructor ( node, module, start, end ) { this.node = node; @@ -169,11 +154,6 @@ export default class Statement { // /update expressions) need to be captured let writeDepth = 0; - // Used to track and optimize lookups through namespaces. - let localName; // The local name of the top-most imported namespace. - let topNode = null; // The top-node of the member expression. - let namespace = null; // An instance of `Module`. - if ( !this.isImportDeclaration ) { walk( this.node, { enter: ( node, parent ) => { @@ -190,93 +170,6 @@ export default class Statement { if ( /Function/.test( node.type ) && !isIife( node, parent ) ) readDepth -= 1; if ( node._scope ) scope = scope.parent; - - // Optimize namespace lookups, which manifest as MemberExpressions. - if ( node.type === 'MemberExpression' && ( !topNode || node.object === topNode ) ) { - // Ignore anything that doesn't begin with an identifier. - if ( !topNode && node.object.type !== 'Identifier') return; - - topNode = node; - - // If we don't already have a namespace, - // we aren't currently exploring any chain of member expressions. - if ( !namespace ) { - localName = node.object.name; - - // At first, we don't have a namespace, so we'll try to look one up. - const id = this.module.locals.lookup( localName ); - - // It only counts if it exists, is a module, and isn't external. - if ( !id || !id.isModule || id.isExternal ) return; - - namespace = id; - } - - // If a namespace is the left hand side of an assignment, throw an error. - if ( parent.type === 'AssignmentExpression' && parent.left === node || - parent.type === 'UpdateExpression' && parent.argument === node ) { - const err = new Error( `Illegal reassignment to import '${chainedMemberExpression( node )}'` ); - err.file = this.module.id; - err.loc = getLocation( this.module.magicString.toString(), node.start ); - throw err; - } - - // Extract the name of the accessed property, from and Identifier or Literal. - // Any eventual Literal value is converted to a string. - const name = !node.computed ? node.property.name : - ( node.property.type === 'Literal' ? String( node.property.value ) : null ); - - // If we can't resolve the name being accessed statically, - // we mark the whole namespace for inclusion in the bundle. - // - // // resolvable - // console.log( javascript.keywords.for ) - // console.log( javascript.keywords[ 'for' ] ) - // console.log( javascript.keywords[ 6 ] ) - // - // // unresolvable - // console.log( javascript.keywords[ index ] ) - // console.log( javascript.keywords[ 1 + 5 ] ) - if ( name === null ) { - namespace.mark(); - - namespace = null; - topNode = null; - return; - } - - const id = namespace.exports.lookup( name ); - - // If the namespace doesn't export the given name, - // we can throw an error (even for nested namespaces). - if ( !id ) { - throw new Error( `Module "${namespace.id}" doesn't export "${name}"!` ); - } - - // We can't resolve deeper. Replace the member chain. - if ( parent.type !== 'MemberExpression' || !( id.isModule && !id.isExternal ) ) { - if ( !~this.dependantIds.indexOf( id ) ) { - this.dependantIds.push( id ); - } - - // FIXME: do this better - // If an earlier stage detected that we depend on this name... - if ( this.dependsOn[ localName ] ) { - // ... decrement the count... - if ( !--this.dependsOn[ localName ] ) { - // ... and remove it if the count is 0. - delete this.dependsOn[ localName ]; - } - } - - this.namespaceReplacements.push( [ topNode, id ] ); - namespace = null; - topNode = null; - return; - } - - namespace = id; - } } }); } diff --git a/src/optimise/namespace-lookup.js b/src/optimise/namespace-lookup.js new file mode 100644 index 0000000..ca03fde --- /dev/null +++ b/src/optimise/namespace-lookup.js @@ -0,0 +1,114 @@ +import walk from '../ast/walk.js'; +import getLocation from '../utils/getLocation.js'; + +// Extract the property access from a MemberExpression. +function property ( node ) { + return node.name ? `.${node.name}` : `[${node.value}]`; +} + +// Recursively traverse the chain of member expressions from `node`, +// returning the access, e.g. `foo.bar[17]` +function chainedMemberExpression ( node ) { + if ( node.object.type === 'MemberExpression' ) { + return chainedMemberExpression( node.object ) + property( node.property ); + } + + return node.object.name + property( node.property ); +} + +export default function ( statement ) { + let localName; // The local name of the top-most imported namespace. + let topNode = null; // The top-node of the member expression. + let namespace = null; // An instance of `Module`. + + walk( statement.node, { + leave ( node, parent ) { + // Optimize namespace lookups, which manifest as MemberExpressions. + if ( node.type === 'MemberExpression' && ( !topNode || node.object === topNode ) ) { + // Ignore anything that doesn't begin with an identifier. + if ( !topNode && node.object.type !== 'Identifier') return; + + topNode = node; + + // If we don't already have a namespace, + // we aren't currently exploring any chain of member expressions. + if ( !namespace ) { + localName = node.object.name; + + // At first, we don't have a namespace, so we'll try to look one up. + const id = statement.module.locals.lookup( localName ); + + // It only counts if it exists, is a module, and isn't external. + if ( !id || !id.isModule || id.isExternal ) return; + + namespace = id; + } + + // If a namespace is the left hand side of an assignment, throw an error. + if ( parent.type === 'AssignmentExpression' && parent.left === node || + parent.type === 'UpdateExpression' && parent.argument === node ) { + const err = new Error( `Illegal reassignment to import '${chainedMemberExpression( node )}'` ); + err.file = statement.module.id; + err.loc = getLocation( statement.module.magicString.toString(), node.start ); + throw err; + } + + // Extract the name of the accessed property, from and Identifier or Literal. + // Any eventual Literal value is converted to a string. + const name = !node.computed ? node.property.name : + ( node.property.type === 'Literal' ? String( node.property.value ) : null ); + + // If we can't resolve the name being accessed statically, + // we mark the whole namespace for inclusion in the bundle. + // + // // resolvable + // console.log( javascript.keywords.for ) + // console.log( javascript.keywords[ 'for' ] ) + // console.log( javascript.keywords[ 6 ] ) + // + // // unresolvable + // console.log( javascript.keywords[ index ] ) + // console.log( javascript.keywords[ 1 + 5 ] ) + if ( name === null ) { + namespace.mark(); + + namespace = null; + topNode = null; + return; + } + + const id = namespace.exports.lookup( name ); + + // If the namespace doesn't export the given name, + // we can throw an error (even for nested namespaces). + if ( !id ) { + throw new Error( `Module "${namespace.id}" doesn't export "${name}"!` ); + } + + // We can't resolve deeper. Replace the member chain. + if ( parent.type !== 'MemberExpression' || !( id.isModule && !id.isExternal ) ) { + if ( !~statement.dependantIds.indexOf( id ) ) { + statement.dependantIds.push( id ); + } + + // FIXME: do this better + // If an earlier stage detected that we depend on this name... + if ( statement.dependsOn[ localName ] ) { + // ... decrement the count... + if ( !--statement.dependsOn[ localName ] ) { + // ... and remove it if the count is 0. + delete statement.dependsOn[ localName ]; + } + } + + statement.namespaceReplacements.push( [ topNode, id ] ); + namespace = null; + topNode = null; + return; + } + + namespace = id; + } + } + }); +} diff --git a/test/function/namespace-optimisation-before-exports/_config.js b/test/function/namespace-optimisation-before-exports/_config.js new file mode 100644 index 0000000..e730b5a --- /dev/null +++ b/test/function/namespace-optimisation-before-exports/_config.js @@ -0,0 +1,5 @@ +module.exports = { + description: 'namespace optimisation must be done after all exports are defined' +}; + +// See: https://github.com/rollup/rollup/issues/148 diff --git a/test/function/namespace-optimisation-before-exports/bar.js b/test/function/namespace-optimisation-before-exports/bar.js new file mode 100644 index 0000000..c2b6db8 --- /dev/null +++ b/test/function/namespace-optimisation-before-exports/bar.js @@ -0,0 +1 @@ +export default function bar () {} diff --git a/test/function/namespace-optimisation-before-exports/foo.js b/test/function/namespace-optimisation-before-exports/foo.js new file mode 100644 index 0000000..d4e76e2 --- /dev/null +++ b/test/function/namespace-optimisation-before-exports/foo.js @@ -0,0 +1 @@ +export { default as bar } from './bar.js'; diff --git a/test/function/namespace-optimisation-before-exports/main.js b/test/function/namespace-optimisation-before-exports/main.js new file mode 100644 index 0000000..85eac4c --- /dev/null +++ b/test/function/namespace-optimisation-before-exports/main.js @@ -0,0 +1,6 @@ +import * as foo from './foo'; +import './zoo'; + +export default { + foo: foo +}; diff --git a/test/function/namespace-optimisation-before-exports/zoo.js b/test/function/namespace-optimisation-before-exports/zoo.js new file mode 100644 index 0000000..82a9c9d --- /dev/null +++ b/test/function/namespace-optimisation-before-exports/zoo.js @@ -0,0 +1,7 @@ +import * as foo from './foo'; + +function wah () { + foo.bar(); +} + +wah();