Improves comment handling in CommonJS import transformation

README.md

@@ -45,8 +45,6 @@ Our transforms will automatically distinguish and pass through Recast config key
 
 ## Known issues
 
-* Currently loses comments if directly before the `require()` statement.
-* `require()` calls in single var statements get reordered, and moved before the single var after conversion to import.
 * Can't automagically figure out when you want to use `import * as varName`.
 * End-of-line comments also missing in many situations
 * `simple-arrow` loses comments in the function expression body

test/fixtures/cjs/comments.after.js

@@ -0,0 +1,21 @@
+/**
+ * External dependencies
+ */
+import React from 'react';
+
+import SomeImport from 'some-import/main';
+import omit from 'lodash/omit';
+
+/**
+ * Internal dependencies
+ */
+import Fetcher from 'components/list-fetcher';
+
+import Page from './page';
+import infiniteScroll from 'mixins/infinite-scroll';
+import observe from 'mixins/observe';
+import EmptyContent from 'components/empty-content';
+import NoResults from 'no-results';
+import actions from 'actions';
+import Placeholder from './placeholder';
+import { mapPostStatus as mapStatus } from 'lib/route';

test/fixtures/cjs/comments.before.js

@@ -0,0 +1,19 @@
+/**
+ * External dependencies
+ */
+var React = require( 'react' ),
+	SomeImport = require( 'some-import/main' ),
+	omit = require( 'lodash/omit' );
+
+/**
+ * Internal dependencies
+ */
+var Fetcher = require( 'components/list-fetcher' ),
+	Page = require( './page' ),
+	infiniteScroll = require( 'mixins/infinite-scroll' ),
+	observe = require( 'mixins/observe' ),
+	EmptyContent = require( 'components/empty-content' ),
+	NoResults = require( 'no-results' ),
+	actions = require( 'actions' ),
+	Placeholder = require( './placeholder' ),
+	mapStatus = require( 'lib/route' ).mapPostStatus;

test/transforms/cjs.js

@@ -24,6 +24,7 @@ describe('CJS transform', function() {
   it('var ... = require("y")( ... )', helper.bind(this, 'called'))
   it('var x = { x: require("x"), y: require("y"), ... }', helper.bind(this, 'mapper'))
   it('should ignore requires deepr than top-level', helper.bind(this, 'ignore'))
+  it('should preserve comments before and after require\'s', helper.bind(this, 'comments'))
 });
 
 function helper (name) {

transforms/cjs.js

@@ -2,6 +2,7 @@
  * cjs - Replace require() calls with es6 imports statements
  */
 
+ var sortBy = require('lodash/sortBy');
 var util = require('../utils/main');
 
 module.exports = function transformer(file, api, options) {
@@ -13,15 +14,21 @@ module.exports = function transformer(file, api, options) {
 		.forEach(function (expressionStatement) {
 			if (!isParentRoot(expressionStatement)) return
 			j(expressionStatement).replaceWith(convertRequire(expressionStatement, expressionStatement.node.comments));
-		})
+		});
 
-	// var ... = require('y')
-	root.find(j.VariableDeclarator, { init: { callee: { name: 'require' }}})
-		.forEach(replaceDeclarator.bind(undefined, j))
+	root.find(j.VariableDeclaration).forEach(function(variableDeclaration) {
+		// var ... = require('y')
+		var defaultImports = j(variableDeclaration).find(j.VariableDeclarator, { init: { callee: { name: 'require' }}});
 
-	// var ... = require('y').x
-	root.find(j.VariableDeclarator, { init: { object: { callee: { name: 'require' }}}})
-		.forEach(replaceDeclarator.bind(undefined, j))
+		// var ... = require('y').x
+		var namedImports = j(variableDeclaration).find(j.VariableDeclarator, { init: { object: { callee: { name: 'require' }}}});
+
+		var sortedImports = sortBy(
+			defaultImports.paths().concat(namedImports.paths()),
+			['value.loc.start.line', 'value.loc.start.column']
+		);
+		sortedImports.forEach(replaceDeclarator.bind(undefined, j))
+  });
 
 	// var x = { x: require('...'), y: require('...'), ... }
 	root.find(j.VariableDeclaration, { declarations: [{ init: { type: 'ObjectExpression' }}] })
@@ -43,8 +50,8 @@ module.exports = function transformer(file, api, options) {
 
 					j(variableDeclaration).insertBefore(importStatement)
 					j(property).replaceWith(newProp)
-				})
-		})
+				});
+		});
 
 	// var x = require("y")( ... )
 	root.find(j.VariableDeclarator, {
@@ -65,7 +72,7 @@ module.exports = function transformer(file, api, options) {
 }
 
 function isParentRoot(path) {
-  return path.parent.node.type === 'Program'
+  return path.parent.node.type === 'Program';
 }
 
 function convertRequire (ast, comments) {
@@ -78,20 +85,25 @@ function convertRequire (ast, comments) {
 	);
 }
 
-function replaceDeclarator (j, variableDeclarator) {
+function replaceDeclarator (j, variableDeclarator, index) {
 	var variableDeclaration = variableDeclarator.parent
+	var variableDeclarationComments = Array.isArray(variableDeclaration.node.comments) ? variableDeclaration.node.comments : []
 	if (!isParentRoot(variableDeclaration)) return
 
 	// create unique variableDeclaration for each declarator (for more consistent prop extraction)
 	var varStatement = j.variableDeclaration('var', [variableDeclarator.node]);
-	var isSingleDeclarator = variableDeclaration.node.declarations.length === 1
-
-	if (isSingleDeclarator) {
-		j(variableDeclaration).replaceWith(convertRequire(varStatement, variableDeclaration.node.comments))
+	var isLastDeclarator = variableDeclaration.node.declarations.length === 1
+	var isFirstDeclarator = index === 0;
+	var comments = variableDeclarationComments.filter(function(comment){
+		return (isFirstDeclarator && comment.leading) || (isLastDeclarator && comment.trailing);
+	})
+
+	if (isLastDeclarator) {
+		j(variableDeclaration).replaceWith(convertRequire(varStatement, comments));
 	} else {
 		// HACK: Using before for now, to avoid it mangling the whitespace after the var statement.
 		// This will cause problems if the single var statement contains deps that the other els depend on
-		j(variableDeclaration).insertBefore(convertRequire(varStatement));
+		j(variableDeclaration).insertBefore(convertRequire(varStatement, comments));
 		j(variableDeclarator).remove();
 	}
 }