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

Ajax fetch error fixes #2064

Merged
merged 6 commits into from
Aug 30, 2023
Merged

Conversation

koddsson
Copy link
Contributor

@koddsson koddsson commented Aug 24, 2023

What I did

  1. I set the name property of the error so that the name is preserved. Without setting the name property it's overwritten when calling super.

Input

class MyError extends Error {
    constructor() {
        super('My error!')
    }
}
try { throw new MyError('foobar') } catch(err) { console.log(err.name) }

Output

Error

Compare that with the same code where we actually set the name property:

Input

class MyError extends Error {
    constructor() {
        super('My error!')
        this.name = 'MyError'
    }
}
try { throw new MyError('foobar') } catch(err) { console.log(err.name) }

Output

MyError
  1. Added the response status code and text to the message.

@changeset-bot
Copy link

changeset-bot bot commented Aug 24, 2023

🦋 Changeset detected

Latest commit: 38e1f36

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@lion/ajax Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@CLAassistant
Copy link

CLAassistant commented Aug 24, 2023

CLA assistant check
All committers have signed the CLA.

@koddsson
Copy link
Contributor Author

@tlouisse
Copy link
Member

And what do you think about linting for this in the future?

https://github.com/sindresorhus/eslint-plugin-unicorn/blob/6d15a02d48de7ecfc38d0683a8487b2f937d83a0/docs/rules/custom-error-definition.md

Seems like something that you would easily forget, so good idea to add the rule. Would you like to add a PR for it?

@koddsson
Copy link
Contributor Author

@tlouisse; I think this latest change should be good but CI isn't running. You probably need to push the button again :)

@tlouisse
Copy link
Member

@tlouisse; I think this latest change should be good but CI isn't running. You probably need to push the button again :)

Awesome. We will merge it soon to master (master is currently in a 'prerelease mode', it will be merged when it's back in 'normal mode') and let you know when it will be released on npm.

@tlouisse tlouisse merged commit 55d6c75 into ing-bank:master Aug 30, 2023
4 checks passed
@koddsson koddsson deleted the ajax-fetch-error-fixes branch August 31, 2023 19:33
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.

3 participants