Move --source-dir coercion into its option configuration

[kumar303]

The --source-dir path needs to get coerced into a resolved path. We had a hack in place to do this while we waited on yargs/yargs#535. Let's remove the hack!

The old way was to coerce the value in run().

The new way can be to use the .coerce() callback. We upgraded yargs for this feature in #435

[shubheksha]

For this, I figured that I'll need to make changes here. However, on going through the docs, it isn't very clear how it'll be implemented as opposed to the way alternate usage options have been given for choices.
@rpl, can you give me some direction on this issue?

[rpl]

@rpl, can you give me some direction on this issue?
@shubheksha sure, it seems that this new yargs coerce feature can be used to transform a value passed to a yargs option, to be able to try this change we need to:

• remove the old code that is currently handling this transformation: https://github.com/mozilla/web-ext/blob/master/src/program.js#L82-L86
• define a coerce function for the sourceDir option, using one of the supported syntax, e.g.

Defined as a property of the option config object:

program.setGlobalOptions({
    'source-dir': {
      alias: 's',
      ...
      type: 'string',
      coerce: (value) => {
         let newvalue = ....;
         ...;
         return newvalue;
      }
},

or using the coerce method:

...
program.yargs.coerce('sourceDir', (value) => { ... });
...

For this PR we just need to transform the value using path.resolve, and so our coerce definition is probably even simpler of the above snippets.

[shubheksha]

@rpl, thanks, got it! I was wondering if the syntax in the first snippet was valid or no even though it made sense in my head -- this cleared it up. I'll open a PR with the required changes soon.

[shubheksha]

@rpl, I made all the required changes & everything works fine except for test which fails repeatedly & I am not being able to figure out why. This line isn't giving the required output. I've attached the screenshot for reference.

Why is the path being resolved to just .. & not what's required?

[rpl]

@shubheksha I took a look and the following is the reason of the above failure:
currently the conversion of the sourceDir path into an absolute dir is done in the run method of the Program class, but with the change that you are preparing the conversion will be done in the main function exported from the same source file (which then calls program.run as its last step: https://github.com/mozilla/web-ext/blob/master/src/program.js#L279), and so we need to rewrite that test case as a test of main instead of run (and move the test in the right group: https://github.com/mozilla/web-ext/blob/master/tests/test.program.js#L232).

[wagnerand]

Removing 'maybe good first bug' since someone is already working on it.

[shubheksha]

@rpl, I am confused about what you said regarding the modification of the test. From what I understood, currently, the test resides in the program.main group, which according to what you said is it's rightful place since the test belongs to the main function itself. The test calls the main method of the program.js which in turn calls program.run. Am I missing something here?

[rpl]

@shubheksha my apologies, I think that you are right, initially I've been tricked by our mocks in this pretty long test file, but I tried to make the above change and the coerce function is definitely called, nevertheless the argv.sourceDir doesn't seem to be affected by the change.

I'm going to try coerce in a smaller nodejs script to check if it's an issue related to how we are using yargs.

[shubheksha]

@rpl, alright! :) I had a question - how did you test whether coerce function is being called or not? I wanted to step through the code as well to see what's happening, but I was clueless how to do it. I've debugged JS in the browser, but is there a tool which I can use to debug JS outside of it too?

[rpl]

@shubheksha that is a very good question, there are different debugging strategies that can be used to debug a nodejs application and check if it is executing (and who is calling it) a particular part of our application:

• the simplest is adding some logging (temporarily) to be able to visualize how the data is flowing in our application based on this "tracing" logs (this seems a very crude debugging strategies, but with asynchronous code is sometimes the best and fastest option)
• the more complete way to debug it properly is running the application in debugging mode, maybe adding some debugger statement where we want to be sure that the debugger will pause.

e.g. we can apply the following temporary changes (something that will not be part of the final patch):

diff --git a/src/program.js b/src/program.js
index 8cda452..09b7c1b 100644
--- a/src/program.js
+++ b/src/program.js
@@ -79,6 +79,7 @@ export class Program {
         this.yargs.exitProcess(this.shouldExitProgram);
         argv = this.yargs.argv;

+        debugger; // <= This will stop the debugger where the sourceDir value is 
+                         //       supposed to be already resolved
         // Fix up sourceDir. This can hopefully move to option configuration
         // soon. See https://github.com/yargs/yargs/issues/535
         if (argv.sourceDir) {
@@ -157,6 +158,10 @@ Example: $0 --help run.
       normalize: true,
       requiresArg: true,
       type: 'string',
+      coerce: (value) => {
+        debugger; // <= This will pause the debugger every time this function is executed 
+        return path.resolve(value);
+      }
     },
     'artifacts-dir': {
       alias: 'a',

Once we made the above changes we can run the single test that we are interesting in (which will reduce the amount of noise that we need to filter out to focus on the issue we want to debug) with the debug repl enabled:

$ ./node_modules/.bin/mocha debug -r babel-core/register tests/test.program.js -f "sourceDir"   

This command is composed of:

• ./node_modules/.bin/mocha is the test runner executable
• debug is a mocha command which run the tests with the debugging repl enabled
• -r babel-core/register is a mocha option, needed to transpile es6 into es5 on the fly
• tests/test.program.js is the test file that we want to run
• -f "sourceDir" is a mocha option which filter the tests and execute only the ones that match the string

Once we run the above command we get the debugger prompt and the nodejs application paused on the mocha sources:

< Debugger listening on port 5858
connecting to 127.0.0.1:5858 ... ok
break in node_modules/mocha/bin/_mocha:7
  5  */
  6
> 7 var program = require('commander'),
  8   path = require('path'),
  9   fs = require('fs'),
debug>

Using the repl command "c" (and pressing return), we will continue the execution until the this debugger statement has been reached:

debug> c
<   program.main
break in src/program.js:197
 195       type: 'string',
 196       coerce: function coerce(value) {
>197         debugger;
 198         return _path2.default.resolve(value);
 199       }
debug> repl
Press Ctrl + C to leave debug repl
> console.log(value, "=>", _path2.default.resolve(value))
< .. => /zfs-mordor/work/webext/tools
>

Now that we are in our coerce function (the version transpiled from es6 to es5 to be honest), we can use the "repl" command to get a regular nodejs repl that is executing code in the context of the above paused execution.

Or for the simpler approach we can add a temporary console.log where we put the debugger statement in the above examples, execute the test and look and the output of the tracing in the console.

PS: another debugging trick that is useful to know is how to use an Error instance to get the current stack and print it into the tracing log, e.g. something like:

      coerce: (value) => {
        console.log("CANARY", {value, coerced: path.resolve.value, calledFrom: Error().stack});
        return path.resolve(value);
      }

[rpl]

@shubheksha I've been able to isolate the source of the issue, it was way simpler that it seems but it wasn't so easy to debug and spot :-D

as an initial hint:
one of the other properties that we are using in the sourceDir option definition doesn't play well with the coerce function, and after removing that single property the coerce function finally works as expected and sourceDir is resolved as expected.

PS: I'm giving you only the initial hint, so that you can experiment a bit with the nodejs debugging techniques, but let me know if you need more hints about this issue, there is no need to turn it into a frustrating experience, software is written together and it is ok to solve some issues in pairing ;-)

[shubheksha]

@rpl, thank you so much for such a detailed & informative reply! I had no idea this could be done. And I really appreciate the fact that you didn't tell me the solution right away. I really want to try & figure things out on my own as I learn the most that way. :)
I was trying to use the debug option, but I can't reach the breakpoint using the "c" command as you mentioned above, instead this happens:

Also - I had to install mocha globally in order to use it. There was nothing in my node_modules/.bin/ folder so I couldn't use the command you specified to run the test. :/

[shubheksha]

So I managed to figure out that the conflicting property was normalize, but I don't really understand why. Could you please elaborate on that? I'd still like to know why I couldn't run the tests & step through the code to see what's happening.