@saintsebastian a process.exit(...) when using this package as a library is super annoying (as well as logging on the console), we can throw an Error instance from here and move the process.exit(...) and the logging in the CLI wrapper that we have in "bin/create-webextension", how that sounds to you?

Nit, how about mandatory instead of compulsory?

that's the word i was looking for!

@saintsebastian This is definitely better for being able to use the module as a library but we still need to make the following tweaks:

• we want to be sure that the CLI command exits with an "error exit code" (e.g. using process.exit explicitly) when there we catch an error here
• we want to be sure that we also produce a stack trace when the caught error is not expected (which currently
  means "anything but an 'EEXIST' error"), and we can do it by just re-throwing the caught error
• The expected errors (currently only the one above) should not print a stack trace and they should be possibly be
  printed using the color red

To achieve these behaviors we can use the same strategy that we are using in web-ext:

• define a custom UsageError class, and throw an instance of this class for the "expected" errors
• then, in the bin wrapper, check for the errors that are instanceof the UsageError and change the behavior
  based on it
• just re-throw the "unexpected" errors, so that they will be logged with the stack trace in the console

@saintsebastian this test looks great, but we need also one for the opposite scenario: it should re-throws instances of other errors.

@rpl wrote simple version of the second test, web-ext test have makeSureItFails helper to avoid accidentally passing. Should we implement some simple version of that for the future?

@saintsebastian have you tried to declare the class without overriding the constructor with its default implementation? (instead of using the eslint-disable-next-line comment)

@saintsebastian no need to an else block here (if you enter the if block you are going to return in any case. (that would probably "make eslint happy again" :-))

@saintsebastian process.exit(1) seems a bit extreme here in a reusable helper, it will make the application to potentially exit immediatelly everywhere we use this helper, we should just throw error; here and let the caller handle the re-throw exception as needed (e.g. logging the error stack and process.exit(1)).

@saintsebastian we should probably return a more useful object to the caller, e.g. something like:

.then(() => {
  const projectCreatedMessage = getProjectCreatedMessage(projectPath);

  return {projectPath, projectCreatedMessage};
});

This should also make much more clear what line 16 of the bin/create-webextension is actually doing.

@saintsebastian we can omit this catch, it is basically doing what it is going to happen if we omit it.

@saintsebastian it would be better to use console.error here (so that the logs are going to be printer in the stderr instead of stdout).

also, after this catch we should handle the other errors by chaining another catch:

  .catch(onlyInstancesOf(UsageError, (error) => {
    ...
  })
  .catch((error) => {
    console.error(`${chalk.red(error.message)}: ${error.stack}\n`); // or something similar that prints both the message and the stack
    process.exit(1);
  });

@rpl wanted to avoid chaining to catches, so thanks for making this clear.

@saintsebastian this should be path.join(__dirname, "..", "..", "bin) (so that nodejs can add the right path separator based on the current OS)

@saintsebastian we could probably export the getProjectCreatedMessage from the index.js module and use it to generate the expected message here and make this test less verbose (and not affected by changes to the message)

Then, we can test the exported getProjectCreatedMessage on its own (possibly by using expect.stringMatching or expect(...).toMatch(...))

I think that it would be better to assert expect(statRes.isFile()).toBeTruthy() (e.g. because by not being a directory we are also including symbolic links, char and block devices, socket files etc.)

we need to be sure that this test fails if it does not raise an error when the directory exists (and check that we have received the expected error message).

@rpl this time i tried to achieve this using jest own methods, take a look maybe there is no need at all to have onlyInstanceOf.

Why are we raising an exception from this helper?
I would have expected that this test would just try to run the comment when the project directory exists, am I reading it wrong?

there should be no need to turn the export into a single module.exports assignment, we should be able to export them one by one with exports.main = main, export.asciiLogo = ... etc.

uhm... I'm particularly not thrilled about adding an fs.readFileSync call here, especially considering about this module used as a library, it basically means that the nodejs app that includes this module is going to "pause everything else" until it has loaded the asciiLogo.

is exporting the asciiLogo actually needed? we can discuss about why we need it and find another solution for it.

@rpl I agree this is not ideal, i decided to do that because __dirname is different for tests, but now I just pass correct directory.

Do we need all this options here?
I think we should evaluate these options and decide if it makes sense for an app that includes this module as a library to redefine this helpers or if we need them only for testing reasons.

If they are only needed for testing reasons, with jest we can probably just move them in another module file and use the Jest Mocks feature to mock them (http://facebook.github.io/jest/docs/manual-mocks.html)

@rpl i used auto mocking in this case, I think it acheives what you mean