Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

6twenty
Copy link

@6twenty 6twenty commented Aug 14, 2022

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: db58761

This is in keeping with what looks like the original intention of the Coffeescript source file - that only NoTargetErrors are caught, and other errors are thrown.
The way that coffeescript compiles the code means that NoTargetError does not actually act as its own class at all, so `error instanceof NoTargetError` always returns `false`.

Since the original error handling code did not raise any exceptions at all (regardless if it was a NoTargetError or not), I've done the same here so that the functionality is identical (save for not reassigning NoTargetError).
@6twenty
Copy link
Author

6twenty commented Aug 15, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant