segment-boneyard / nightmare

A high-level browser automation library.
https://open.segment.com
19.55k stars 1.08k forks source link

.catch() needs to be better documented #1087

Closed rinogo closed 7 years ago

rinogo commented 7 years ago

I struggled with this one for longer than necessary. My NightmareJS scripts seemed to be hanging for some reason. Even running them with the DEBUG environment option didn't help in determining what the problem was.

As it turns out, NightmareJS wasn't hanging; it was just in an error state, and the script hadn't terminated the Electron instance, so it appeared like NightmareJS had hung.

The big "A ha!" moment was realizing that I needed to be using .catch() instead of some other mechanism like try {} catch() {}. This may be obvious to some, but to me, it wasn't. It was especially frustrating because .catch() isn't even mentioned in the API documentation. In fact, the only place I've found it mentioned is in a few NightmareJS examples.

I'd recommend two changes;

  1. Adding .catch() to the API documentation (i.e. Readme.md)
  2. Upgrading all examples to include a .catch() call, if only to instill from the very beginning the necessity of error handling (and the right way to do it).
rinogo commented 7 years ago

Also, if it helps anyone, here's the .catch() block I'm currently using.

overflowz commented 7 years ago

.catch() and .then() are member functions of promise. it has no relation to nightmare itself.

rinogo commented 7 years ago

Thanks for the clarification! Regardless, regarding the second point, wouldn't the examples be better, more comprehensive examples if they included .catch() calls?

Not doing so is a bit like doing file I/O in something like Ruby or PHP without checking for errors/exceptions. Yes, it should generally work, but the resulting code will be buggy and unreliable.

Veterans, of course, will know that they need to include the appropriate exception handling. However, the target audience of documentation and examples are usually not novices, not veterans.

overflowz commented 7 years ago

Examples are written as a quick demonstration without error handling. catch() is just a basics of promise. Full example is provided here with catch(): https://github.com/segmentio/nightmare#examples

casesandberg commented 7 years ago

We are currently working on some better documentation, but as @overflowz pointed out, catch is from the promise.

rinogo commented 7 years ago

Thank you. Any thoughts on my second recommendation?

Upgrading all examples to include a .catch() call, if only to instill from the very beginning the necessity of error handling (and the right way to do it).

casesandberg commented 7 years ago

Yes! That will be in the new documentation

overflowz commented 7 years ago

@rinogo I'd really suggest you to have a look at promises, how they are working, what they do and so on, you'll get better understanding of them. I'm telling this, because mostly nobody documents third-party library (or native javascript) features in their libraries, only the basics.

For example me, instead of doing the promise callback chain with then() and catch(), I'm using new async/await style, which would look like this:

async function run() {
  try {
    await nightmare.goto(...);
    await nightmare.type(...);
  } catch (err) {
    console.error(err);
  }
}
...

which is equal to:

function run() {
  nightmare.goto(...).then(() => nightmare.type(...)).catch(err => console.error(err));
}

Regards.

rinogo commented 7 years ago

Hi, @overflowz, thanks for your comment. Since April 17, I have, indeed, become more familiar with Promises. :) Regardless, I think the recommendation to have a consistent error handling approach presented in the examples/documentation is a reasonable recommendation.

overflowz commented 7 years ago

@rinogo It is, indeed. I just pointed out how many documentations are made (because they're written fast and missing many things). I just gave you suggestion, which you already know now, good to know though!

Have a nice day though, I'm bit tired at the moment so, apologies!

rinogo commented 7 years ago

Thanks for your comments and help - have a great day and get some sleep soon! :)