CodeByZach / pace

Automatically add a progress bar to your site.
https://codebyzach.github.io/pace/
MIT License
15.68k stars 1.9k forks source link

Fix a bug when there's an error when starting or destroying the bar #538

Open 6twenty opened 2 years ago

6twenty commented 2 years ago

Presumably this is incredibly unlikely to happen as I couldn't see any issues about this, and it's a bug that has been around for a very long time.

There's an error class that gets defined, NoTargetError, that inherits from Error and is used in Bar.prototype.getElement when the target element doesn't exist in the DOM. However, there's a couple of places in the code where NoTargetError gets redefined inside of catch {} blocks, and assigned to the error instance instead. Later, if the code tries to throw new NoTargetError again, it'll fail because the error instance is not a constructor.

I looked back at an older version of the repo to find the original Coffeescript source file: https://github.com/CodeByZach/pace/blob/v1.2.0/pace.coffee. I believe that the error handling in this file was just poorly written - they've used catch NoTargetError, and I believe the intention here is that only NoTargetError errors should be caught. But the resulting compiled javascript code is:

catch (_error) {
  NoTargetError = _error;
}

I've re-written those catch blocks as:

catch (_error) {
  // This is ok
}

This means that all errors, including NoTargetError, will get caught and suppressed - the same behaviour as before. But the code no longer reassigns NoTargetError.

The original commit introducing NoTargetError and the catch block is here: https://github.com/CodeByZach/pace/commit/db587611564d06ee8218d9b63567eec121cc0642

6twenty commented 2 years ago

Updated the PR code and description as I discovered that you can't do error instanceof NoTargetError - the way that Coffeescript compiled the original source code means that NoTargetError doesn't act like a class at all, so there's no way to assert that a particular error is an instance of NoTargetError.

The change is now simply to remove the unwanted NoTargetError = _error lines.