feat: update bulk-decaffeinate for decaffeinate 3.0 (#131)

* Add a `--use-js-modules` option that performs the fix-imports step and sets
  the decaffeinate args to use JS modules.
* Update a bunch of tests to reflect the defaults changes.
* Make the `--allow-invalid-constructors` option a no-op, since it isn't needed
  with decaffeinate 3.0.
* Update the README example to no longer use `--allow-invalid-constructors`.
  Also change it from hubot to coffeelint, since hubot is now a JS project.

BREAKING CHANGE: The `--allow-invalid-constructors` option is now a no-op.

README.md

@@ -13,35 +13,42 @@ some follow-up cleanups. Here's an example of checking the Hubot repo:
 ```
 > npm install -g bulk-decaffeinate decaffeinate eslint
 ...
-> git clone git@github.com:github/hubot.git
+> git clone https://github.com/philc/vimium.git
 ...
-> cd hubot
+> cd vimium
 > bulk-decaffeinate check
-Doing a dry run of decaffeinate on 18 files...
-18/18 (5 failures so far)
-5 files failed to convert:
-src/adapter.coffee
-src/adapters/campfire.coffee
-src/brain.coffee
-src/listener.coffee
-src/message.coffee
-
-Wrote decaffeinate-errors.log and decaffeinate-results.json with more detailed info.
-To open failures in the online repl, run "bulk-decaffeinate view-errors".
-To convert the successful files, run "bulk-decaffeinate convert -p decaffeinate-successful-files.txt".
-> bulk-decaffeinate view-errors
-(7 browser tabs are opened, showing all failures.)
-> bulk-decaffeinate check --allow-invalid-constructors
-Doing a dry run of decaffeinate on 18 files...
-18/18
-All checks succeeded! decaffeinate can convert all 18 files.
+Doing a dry run of decaffeinate on 50 files...
+50/50
+All checks succeeded! decaffeinate can convert all 50 files.
 Run "bulk-decaffeinate convert" to convert the files to JavaScript.
+> bulk-decaffeinate convert
+Verifying that decaffeinate can successfully convert these files...
+50/50
+Backing up files to .original.coffee...
+50/50
+Renaming files from .coffee to .js...
+50/50
+Generating the first commit: "decaffeinate: Rename bg_utils.coffee and 49 other files from .coffee to .js"...
+Moving files back...
+50/50
+Running decaffeinate on all files...
+50/50
+Deleting old files...
+50/50
+Setting proper extension for all files...
+50/50
+Generating the second commit: decaffeinate: Convert bg_utils.coffee and 49 other files to JS...
+Running eslint --fix on all files...
+50/50
+[Skips eslint for all files because there is no config.]
+Generating the third commit: decaffeinate: Run post-processing cleanups on bg_utils.coffee and 49 other files...
+Successfully ran decaffeinate on 50 files.
+You should now fix lint issues in any affected files.
+All CoffeeScript files were backed up as .original.coffee files that you can use for comparison.
+You can run "bulk-decaffeinate clean" to remove those files.
+To allow git to properly track file history, you should NOT squash the generated commits together.
 ```
 
-Once any failures are resolved (generally by tweaking the CoffeeScript to work
-with decaffeinate), the command `bulk-decaffeinate convert` generates three git
-commits to convert the files to JS.
-
 ## Assumptions
 
 While the underlying [decaffeinate](https://github.com/decaffeinate/decaffeinate)
@@ -178,19 +185,16 @@ recursively discover all CoffeeScript files in the working directory.
 Each of these has a command line arg version, which takes precedence over config
 file values; see the result of `--help` for more information.
 
-### Other configuration
+### Common configuration options
 
+* `useJSModules`: an optional boolean. If true, decaffeinate will be configured
+  to produce code with `import`/`export` syntax, and the fix-imports step will
+  be run afterward to correct any import statements across the codebase. The
+  fix-imports step can be configured using `fixImportsConfig`.
 * `decaffeinateArgs`: an optional array of additional command-line arguments to
   pass to decaffeinate. For example, `['--keep-commonjs']` sets the preference
   to keep `require` and `module.exports` rather than converting them to `import`
   and `export`.
-* `customNames`: an optional object mapping old filename to new filename. By
-  default, the extension is removed and replaced with ".js" (or nothing for
-  extensionless files), but this mapping can be used to override the behavior
-  to provide a specific target directory, name, and/or file extension for any
-  specific files being converted.
-* `outputFileExtension`: an optional file extension, like `"ts"` or `"jsx"`. If
-  specified, all converted files will have this extension.
 * `jscodeshiftScripts`: an optional array of paths to
   [jscodeshift](https://github.com/facebook/jscodeshift) scripts to run after
   decaffeinate. This is useful to automate any cleanups to convert the output of
@@ -211,6 +215,16 @@ file values; see the result of `--help` for more information.
     as an absolute path starting point when resolving imports. This is necessary
     if you do any tricks to get absolute-style imports in your project, since
     the fix-imports script needs to be able to resolve import names to files.
+
+### Other configuration
+
+* `customNames`: an optional object mapping old filename to new filename. By
+  default, the extension is removed and replaced with ".js" (or nothing for
+  extensionless files), but this mapping can be used to override the behavior
+  to provide a specific target directory, name, and/or file extension for any
+  specific files being converted.
+* `outputFileExtension`: an optional file extension, like `"ts"` or `"jsx"`. If
+  specified, all converted files will have this extension.
 * `mochaEnvFilePattern`: an optional regular expression string. If specified,
   all generated JavaScript files with a path matching this pattern have the text
   `/* eslint-env mocha */` added to the start. For example, `"^.*-test.js$"`.

package.json

@@ -44,7 +44,7 @@
     "babel-preset-es2015": "^6.13.0",
     "babel-register": "^6.11.6",
     "babelrc-rollup": "^3.0.0",
-    "decaffeinate": "^2.55.0",
+    "decaffeinate": "^3.0.0",
     "eslint": "^3.18.0",
     "eslint-plugin-babel": "^4.0.0",
     "jscodeshift": "^0.3.30",

src/cli.js

@@ -41,9 +41,10 @@ export default function () {
     .option('-d, --dir [path]',
       `A directory containing files to decaffeinate. All .coffee files in
                               any subdirectory of this directory are considered for decaffeinate.`)
-    .option('--allow-invalid-constructors',
-      `If specified, the --allow-invalid-constructors arg is added when
-                              invoking decaffeinate.`)
+    .option('--use-js-modules',
+      `If specified, decaffeinate will convert the code to use import/export
+                              syntax and a follow-up fix-imports step will correct any imports
+                              across the codebase.`)
     .option('--land-base [revision]',
       `The git revision to use as the base commit when running the "land"
                               command. If none is specified, bulk-decaffeinate tries to use the
@@ -61,6 +62,8 @@ export default function () {
     .option('--eslint-path [path]',
       `The path to the eslint binary. If none is specified, it will be
                               automatically discovered from node_modules and then from the PATH.`)
+    .option('--allow-invalid-constructors',
+      `Deprecated; decaffeinate now allows invalid constructors by default.`)
     .parse(process.argv);
 
   runCommand(command);

src/config/resolveConfig.js

@@ -27,14 +27,14 @@ export default async function resolveConfig(commander) {
   }
   config = getCLIParamsConfig(config, commander);
   return {
-    decaffeinateArgs: config.decaffeinateArgs || [],
+    decaffeinateArgs: resolveDecaffeinateArgs(config),
     filesToProcess: config.filesToProcess,
     pathFile: config.pathFile,
     searchDirectory: config.searchDirectory,
     fileFilterFn: config.fileFilterFn,
     customNames: resolveCustomNames(config.customNames),
     outputFileExtension: config.outputFileExtension || 'js',
-    fixImportsConfig: config.fixImportsConfig,
+    fixImportsConfig: resolveFixImportsConfig(config),
     jscodeshiftScripts: config.jscodeshiftScripts,
     landConfig: config.landConfig,
     mochaEnvFilePattern: config.mochaEnvFilePattern,
@@ -47,6 +47,24 @@ export default async function resolveConfig(commander) {
   };
 }
 
+function resolveDecaffeinateArgs(config) {
+  let args = config.decaffeinateArgs || [];
+  if (config.useJSModules && !args.includes('--use-js-modules')) {
+    args.push('--use-js-modules');
+  }
+  return args;
+}
+
+function resolveFixImportsConfig(config) {
+  let fixImportsConfig = config.fixImportsConfig;
+  if (!fixImportsConfig && config.useJSModules) {
+    fixImportsConfig = {
+      searchPath: '.',
+    };
+  }
+  return fixImportsConfig;
+}
+
 async function applyPossibleConfig(filename, config) {
   if (!filename.startsWith('bulk-decaffeinate') ||
       (await stat(filename)).isDirectory()) {
@@ -79,7 +97,7 @@ function getCLIParamsConfig(config, commander) {
     file,
     pathFile,
     dir,
-    allowInvalidConstructors,
+    useJsModules,
     landBase,
     skipVerify,
     decaffeinatePath,
@@ -103,8 +121,8 @@ function getCLIParamsConfig(config, commander) {
   if (pathFile) {
     config.pathFile = pathFile;
   }
-  if (allowInvalidConstructors) {
-    config.decaffeinateArgs = [...(config.decaffeinateArgs || []), '--allow-invalid-constructors'];
+  if (useJsModules) {
+    config.useJSModules = true;
   }
   if (landBase) {
     config.landBase = landBase;
@@ -134,7 +152,7 @@ async function resolveDecaffeinatePath(config) {
 async function resolveJscodeshiftPath(config) {
   // jscodeshift is an optional step, so don't prompt to install it if we won't
   // be using it.
-  if (!config.jscodeshiftScripts && !config.fixImportsConfig) {
+  if (!config.jscodeshiftScripts && !config.fixImportsConfig && !config.useJSModules) {
     return null;
   }
   if (config.jscodeshiftPath) {

test/convert-test.js

@@ -82,7 +82,7 @@ Sample User <[email protected]> Initial commit
 */
 // TODO: This file was created by bulk-decaffeinate.
 // Fix any style issues and re-enable lint.
-let a = 1;
+const a = 1;
 `);
       await assertFileContents('./B.js', `\
 /* eslint-disable
@@ -91,7 +91,7 @@ let a = 1;
 // TODO: This file was created by bulk-decaffeinate.
 // Fix any style issues and re-enable lint.
 // This is a literate file.
-let b = 1;
+const b = 1;
 `);
       await assertFileContents('./C.js', `\
 /* eslint-disable
@@ -100,7 +100,7 @@ let b = 1;
 // TODO: This file was created by bulk-decaffeinate.
 // Fix any style issues and re-enable lint.
 // This is another literate file.
-let c = 1;
+const c = 1;
 `);
     });
   });
@@ -114,8 +114,8 @@ let c = 1;
 */
 // TODO: This file was created by bulk-decaffeinate.
 // Fix any style issues and re-enable lint.
-let nameAfter = 3;
-let notChanged = 4;
+const nameAfter = 3;
+const notChanged = 4;
 `);
     });
   });
@@ -129,7 +129,14 @@ let notChanged = 4;
 */
 // TODO: This file was created by bulk-decaffeinate.
 // Fix any style issues and re-enable lint.
-let a = require('./A');
+/*
+ * decaffeinate suggestions:
+ * DS102: Remove unnecessary code created because of implicit returns
+ * DS208: Avoid top-level this
+ * DS209: Avoid top-level return
+ * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
+ */
+const a = require('./A');
 
 // This is a comment
 function f() {
@@ -203,7 +210,7 @@ console.log('This is a file');
 */
 // TODO: This file was created by bulk-decaffeinate.
 // Fix any style issues and re-enable lint.
-let a = require('b');
+const a = require('b');
 module.exports = c;
 `);
     });
@@ -372,17 +379,4 @@ Proceeding anyway.`);
       assert.equal((await exec('git rev-list --count HEAD'))[0].trim(), '4');
     });
   });
-
-  it('allows invalid constructors when specified', async function() {
-    await runWithTemplateDir('invalid-subclass-constructor', async function() {
-      await runCliExpectSuccess('convert --allow-invalid-constructors');
-    });
-  });
-
-  it('does not allow invalid constructors when not specified', async function() {
-    await runWithTemplateDir('invalid-subclass-constructor', async function() {
-      let message = await runCliExpectError('convert');
-      assertIncludes(message, 'Some files could not be converted with decaffeinate');
-    });
-  });
 });

test/examples/fix-imports-absolute-imports/bulk-decaffeinate.config.js

@@ -1,4 +1,5 @@
 module.exports = {
+  decaffeinateArgs: ['--loose-js-modules'],
   fixImportsConfig: {
     searchPath: '.',
     absoluteImportPaths: ['.'],

test/examples/fix-imports-default-import-to-import-star/bulk-decaffeinate.config.js

@@ -1,5 +1,3 @@
 module.exports = {
-  fixImportsConfig: {
-    searchPath: '.',
-  },
+  decaffeinateArgs: ['--loose-js-modules'],
 };

test/examples/fix-imports-destructure-from-import-star/bulk-decaffeinate.config.js

@@ -1,5 +1,3 @@
 module.exports = {
-  fixImportsConfig: {
-    searchPath: '.',
-  },
+  decaffeinateArgs: ['--loose-js-modules'],
 };

test/examples/fix-imports-import-from-existing-js/bulk-decaffeinate.config.js

@@ -1,5 +1,3 @@
 module.exports = {
-  fixImportsConfig: {
-    searchPath: '.',
-  },
+  decaffeinateArgs: ['--loose-js-modules'],
 };

test/examples/fix-imports-named-import-to-destructure/NamedImport.js.expected

@@ -22,7 +22,7 @@ const {
 
 console.log(twelve);
 console.log(tenAndSeven);
-let NameClash = 25;
+const NameClash = 25;
 console.log(twenty);
 console.log(twentySix);
 console.log(NameClash);

test/examples/fix-imports-named-import-to-destructure/bulk-decaffeinate.config.js

@@ -1,5 +1,3 @@
 module.exports = {
-  fixImportsConfig: {
-    searchPath: '.',
-  },
+  decaffeinateArgs: ['--loose-js-modules'],
 };

test/examples/fix-imports-non-relative-path/bulk-decaffeinate.config.js

@@ -1,5 +1,3 @@
 module.exports = {
-  fixImportsConfig: {
-    searchPath: '.',
-  },
+  decaffeinateArgs: ['--loose-js-modules'],
 };

test/fix-imports-test.js

@@ -15,7 +15,7 @@ describe('fix-imports', () => {
     await runWithTemplateDir(dirName, async function () {
       // We intentionally call the files ".js.expected" so that jscodeshift
       // doesn't discover and try to convert them.
-      let {stdout, stderr} = await runCli('convert');
+      let {stdout, stderr} = await runCli('convert --use-js-modules');
       assertIncludes(stdout, 'Fixing any imports across the whole codebase');
       assert.equal(stderr, '');