feat: pass argument to main function by saintsebastian · Pull Request #5 · rpl/create-webextension

saintsebastian · 2017-07-24T16:08:25Z

fixes #3

A very small change, we could expand it further if need arises

rpl · 2017-07-24T17:35:45Z

rpl · 2017-07-24T17:39:04Z

+  if (!dirPath) {
    console.error(`${chalk.red("Missing project dir name.")}\n`);
    console.log(USAGE_MSG);
    process.exit(1);


@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?

saintsebastian · 2017-07-25T17:04:00Z

I am contemplating how to make using the package as a library easier, maybe wrapping current code in bin/create-webextension in a function with optional argument?

rpl

I think that what we moved in the "bin/create-webextension" wrapper is not going to be needed when this package is used as a library, but the main function could probably use some additional parameter (e.g. to configure its behavior when used as a library, a "quite mode" in which this library does not log anything on the console could be really helpful, and also to inject the dependencies and simplify its unit testing).

Also, at line 115 and 119 of the "index.js" module, there are still two process.exit(1), it would be better to turn all these error conditions into rejections of the Promise returned by the main function (which would allow a caller to customize how these error conditions will be handled).

rpl · 2017-07-26T10:47:52Z

-    process.exit(1);
+exports.main = function main(dirPath) {
+  if (!dirPath) {
+    throw new Error("Project directory name is a compulsory argument");


Nit, how about mandatory instead of compulsory?

that's the word i was looking for!

saintsebastian · 2017-07-27T19:11:09Z

@rpl i moved console.log to bin/create-webextenion so main is 'quiet', does that work as intended? i don't know whether adding parameters should be done here or when i'll write test for main

rpl · 2017-07-31T11:05:54Z

-require("..").main(process.argv[2]);
+require("..").main(process.argv[2])
+  .then(console.log)
+  .catch(err => console.log(err.message));


@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 · 2017-08-07T11:53:03Z

@rpl Thanks for the review, i tried to add the behavior you were talking about. In case of onlyInstancesOf i chose to log the trace and exit with code 1 to avoid Unhandled Promise Rejection.
I also added dummy test for onlyInstancesOf to start with.

rpl

Hi @saintsebastian
This looks pretty nice, we need to fix some additional detail, but we are definitely almost there.

Thanks a lot for having started to also create some test units, we should probably move the test files into two subdirectories, e.g. unit and integration, to make it immediately clear the different testing approach).

It seems that we could easily add also a test for the main function itself, what do you think about it?

rpl · 2017-08-08T15:44:11Z

+const onlyInstancesOf = require("../errors").onlyInstancesOf;
+
+describe("onlyInstancesOf", () => {
+  test("catches specified error", () => {


@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?

rpl · 2017-08-08T15:46:03Z

+};
+
+exports.UsageError = class UsageError extends ES6Error {
+  // eslint-disable-next-line  no-useless-constructor


@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)

rpl · 2017-08-08T15:47:08Z

+    if (error instanceof errorType) {
+      return handler(error);
+    // eslint-disable-next-line no-else-return
+    } else {


@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" :-))

rpl · 2017-08-08T15:49:12Z

+    // eslint-disable-next-line no-else-return
+    } else {
+      console.log(error.stack);
+      process.exit(1);


@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)).

rpl · 2017-08-08T15:54:36Z

                                          stripAnsi(projectReadme)))
-      .then(() => getProjectCreatedMessage(projectPath))
-      .then(console.log);
+      .then(() => getProjectCreatedMessage(projectPath));


@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.

rpl · 2017-08-08T15:55:42Z

  }).catch((error) => {
-    console.error(error);
-    process.exit(1);
+    throw error;


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

rpl · 2017-08-08T15:59:59Z

+
+require("..").main(process.argv[2])
+  .then(console.log)
+  .catch(onlyInstancesOf(UsageError, (error) => {


@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 · 2017-08-14T18:33:36Z

hey @rpl! I added some tests and tried to address the thing in your review. Some tests are a bit clunky, so i would appreciate your opinion.

rpl · 2017-08-23T11:41:31Z

+const cmdRunner = require("../helpers/cmd-runner");

-const execDirPath = path.join(__dirname, "..", "bin");
+const execDirPath = path.join(__dirname, "../..", "bin");


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

rpl · 2017-08-23T11:48:03Z

+    async (tmpPath) => {
+      const projName = "target";
+      const targetDir = path.join(tmpPath, projName);
+      const expectedMessage =


@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(...))

rpl · 2017-08-23T11:51:48Z

+      });
+
+      const contentStat = await fs.stat(path.join(targetDir, "content.js"));
+      expect(contentStat.isDirectory()).toBeFalsy();


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.)

rpl · 2017-08-23T11:56:16Z

+          getPlaceholderIconFn: getPlaceholderIconMock,
+        });
+      } catch (error) {
+        expect(error.message).toMatch(/error/);


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.

rpl · 2017-08-23T11:57:40Z

+    async (tmpPath) => {
+      const projName = "target";
+      const getPlaceholderIconMock = jest.fn(() => {
+        throw new Error("error");


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?

rpl · 2017-08-23T11:59:45Z

  });
+}
+
+module.exports = {


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.

rpl · 2017-08-23T12:03:07Z

  ${chalk.bold.blue("web-ext run -s /path/to/extension")}
 `;

+const asciiLogo = fs.readFileSync(


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.

rpl · 2017-08-23T12:06:48Z

+function main({
+  dirPath,
+  baseDir = process.cwd(),
+  getProjectManifestFn = getProjectManifest,


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

feat: pass argument to main function

b2cab46

rpl requested changes Jul 24, 2017

View reviewed changes

move checking for argument into wrapper

12be9ae

saintsebastian force-pushed the dirPath-argument-main branch from b9174fd to 12be9ae Compare July 25, 2017 17:02

rpl requested changes Jul 26, 2017

View reviewed changes

better handled errors

2e41028

rpl requested changes Jul 31, 2017

View reviewed changes

feat: add expected erro class and its handling

63916af

saintsebastian force-pushed the dirPath-argument-main branch from 80ce0fe to 9b35d0e Compare August 7, 2017 11:54

test: initial test for onlyInstancesOf

9b9e16c

saintsebastian force-pushed the dirPath-argument-main branch from 9b35d0e to 9b9e16c Compare August 7, 2017 16:08

rpl requested changes Aug 8, 2017

View reviewed changes

saintsebastian added 3 commits August 10, 2017 22:36

fix: cleanup and better syntax

1457b19

test: initial test for main

441432b

test: unit tests for main

1f936d1

rpl requested changes Aug 23, 2017

View reviewed changes

saintsebastian added 2 commits September 3, 2017 14:50

fix: reviewed tests for index

599396d

test: separate dependencies for main into module

2f52f91

rpl mentioned this pull request Jan 4, 2018

feat: refactoring module main function, apply minor fixes and added testing suite #7

Open

Conversation

saintsebastian commented Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rpl commented Jul 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saintsebastian commented Jul 25, 2017

Uh oh!

rpl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saintsebastian commented Jul 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saintsebastian commented Aug 7, 2017

Uh oh!

rpl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saintsebastian commented Aug 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

saintsebastian commented Jul 24, 2017 •

edited

Loading

rpl left a comment •

edited

Loading