fix(#500): run() callback to noop if missing

[h2non]

Should fix: #500

[rosshinkley]

I don't know that having a .run() call with an internally defined callback makes sense. Consider:

nightmare
  .goto(url)
  .evaluate(someFunction)
  .run();

... which doesn't actually do anything with the results of someFunction. The only cases I can maybe see this being useful is with .screenshot(), .pdf() or .html(). Even at that, it's only the most trivial of cases of each. Is there a usecase I'm not thinking of?

I would advocate for something closer to how #512 works - validating the input and throwing an exception if it's not acceptable.

Thoughts?

[Mr0grog]

The only cases I can maybe see this being useful is with .screenshot(), .pdf() or .html(). Even at that, it's only the most trivial of cases of each.

On the one hand, even the most trivial cases should arguably be accompanied by a callback—you want to handle, or at least be notified of, failures.

However, I think it's useful to make things super simple for someone just getting started with Nightmare and trying things. I also think simply loading a page and taking a screenshot or saving a PDF is a Big Deal of a use case (that’s all I was trying to do when I discovered Nightmare and wound up writing all the frame manager stuff).

I like allowing no callback to be passed to run—with one tweak: the default callback should probably console.error() when errors occur. Otherwise I’d agree with @rosshinkley. Bottom-line, either solution would be much better than the current situation.

[rosshinkley]

The (apparently deleted?) #516 dealt with this... sort of. .run() is really intended for internal use and usage isn't directly supported. Should it be? I feel like that discussion might be a bit off-topic, but I'd like to hear thoughts from you or anyone else.

Back to something more germane:

On the one hand, even the most trivial cases should arguably be accompanied by a callback—you want to handle, or at least be notified of, failures.

Could not agree more, and I guess I was erring on the side of protecting users from themselves, which now I'm realizing might be overstepping a bit. Consider:

nightmare
  .goto(url)
  .screenshot('screenshot.png')
  .end()
  .then();

This is legitimate, and could certainly cause a "silent" (if run without DEBUG) error. Also, consider something like:

nightmare
  .goto(url)
  .wait('body')
  .click('a')
  .end()
  .then();

... which is also completely legitimate. Without context, it doesn't make a whole lot of sense, but now that I think about it, it's possible that you just want to click "OK" on a page in a test suite or something. There are legitimate use cases, I suppose.

I like allowing no callback to be passed to run—with one tweak: the default callback should probably console.error() when errors occur.

... I have to begrudgingly agree. I am not sure I can articulate why I'd prefer input validation over this, but because that's almost precisely how .then() behaves, it's kind of hard to argue.

Bottom-line, either solution would be much better than the current situation.

Yes sir. Something would be better than nothing.

[Mr0grog]

...I have to begrudgingly agree. I am not sure I can articulate why I'd prefer input validation over this, but because that's almost precisely how .then() behaves, it's kind of hard to argue.

I think whether it’s best to throw an error vs. just handle “bad” input is pretty fuzzy, so I totally get you here.

One of my very general rules of thumb is that it’s good to throw an error when “bad” input is meaningless (e.g. sum(3) makes no sense without a second argument) or is ambiguous in its intent. In the case of no callback for run, the intention is still clear and the behavior still has meaning and value.

[rosshinkley]

...the intention is still clear and the behavior still has meaning and value.

Your skilled reasoning has convinced me (twice over, in fact). Coupled with the fact that .then() works that way anyway, I feel like my original exception position is pretty indefensible.

[Mr0grog]

@h2non this patch would be awesome if you want to add some logging of errors to the default fn (unless you disagree with that notion). Thanks for writing it!

[LinusU]

I would prefer throwing the error if no callback function was provided, that's how all the bundled libs with Node.js works (e.g. fs)

[Mr0grog]

that's how all the bundled libs with Node.js work (e.g. fs)

Maybe they used to and no longer do? Or I'm misunderstanding what you meant, @LinusU. I just tried the following in Node 4.4, 5.11, and 6.2; none of them threw an error:

fs.readFile('./README.md', 'utf8'); // where README.md is an actual file
fs.writeFile('./test-test.md', 'testing?');

[rosshinkley]

@LinusU Are you referring to *sync methods and when they except, maybe?

[LinusU]

Sorry for being unclear, I'm referring to the asynchournous version when they are not being given a callback.

e.g.

const fs = require('fs')

fs.readFile('/i/do/not/exist', 'utf8')

running this file will throw an error.

Basically I mean making this the default callback:

function defaultCallback (err) {
  if (err) throw err
}

[Mr0grog]

e.g. fs.readFile('/i/do/not/exist', 'utf8')

True enough! But it does not throw because of the missing callback. The error message refers to the non-existent file. If you run the same code with the path to an existing file, it does not throw.

While I think there are good arguments for throwing, I don't think “built-ins do it” is one of them. Some do (e.g. fs.access) and some don’t (e.g. fs.read/writeFile). When it comes to Node’s standard library, this behavior is inconsistent at best.

[LinusU]

Just to clarify one more time, if no callback is provided and everything goes well, I do not want any error thrown. When no callback is provided and something does go wrong, I want that error to be thrown.

I believe that this behaviour is what all apis in Node.js does.

However, some apis throws an error directly when the function is being called without callback (e.g. fs.access). While I like that behaviour as well, that would be an backwards incompatible change (I think? maybe not).

I guess that there are four levels

1. When no callback is given, don't report any errors
2. When no callback is given, console.error if an error happens
3. When no callback is given, throw if an error happens
4. When no callback is given, throw immediately

What I was saying is that I would rather see 3 than 2 since that is what Node.js builtin api does.

As you correctly point out, Node.js builtin api is split between option 3 and 4. But none of them uses option 1 or 2, and for good reasons, and I don't think that we should either.

[LinusU]

Option 1. 😱

 Nightmare.prototype.run = function(fn) {
+  if (typeof fn !== 'function') {
+    fn = function () {};
+  }
   debug('running')
   var ready = [this.ready.bind(this)];
   var steps = [ready].concat(this.queue());
   this.running = true;

Option 2. 😢

 Nightmare.prototype.run = function(fn) {
+  if (typeof fn !== 'function') {
+    fn = function (err) { if (err) { console.error(err); } };
+  }
   debug('running')
   var ready = [this.ready.bind(this)];
   var steps = [ready].concat(this.queue());
   this.running = true;

Option 3. 👍

 Nightmare.prototype.run = function(fn) {
+  if (typeof fn !== 'function') {
+    fn = function (err) { if (err) { throw err; } };
+  }
   debug('running')
   var ready = [this.ready.bind(this)];
   var steps = [ready].concat(this.queue());
   this.running = true;

Option 4. 👌

 Nightmare.prototype.run = function(fn) {
+  if (typeof fn !== 'function') {
+    throw new TypeError('Expected callback to be a function')
+  }
   debug('running')
   var ready = [this.ready.bind(this)];
   var steps = [ready].concat(this.queue());
   this.running = true;

[rosshinkley]

An important distinction to make: I was originally advocating for input validation, which is too heavy-handed. (Meaning, option 4 isn't going to work.) Throwing on errors when no callback is present after execution is probably worth talking about.

[LinusU]

Why return self?

[matthewmueller]

Closing this as its quite old and we really shouldn't use run much anymore since promises are widespread. Thanks @h2non !