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

chore: remove code duplication in custom errors #842

Merged
merged 2 commits into from Jun 10, 2020
Merged

chore: remove code duplication in custom errors #842

merged 2 commits into from Jun 10, 2020

Conversation

tinovyatkin
Copy link
Member

@tinovyatkin tinovyatkin commented May 27, 2020

In the current implementation AbortError and FetchError pretty much repeating the same code (plus some magic strings). This PR moves common functionality into a base class and removes code duplication.

It also removes isAbortError and uses instanceof FetchBaseError as those errors are our internal classes and always instantiating from it.

@tinovyatkin tinovyatkin changed the title prettier-erros DRY Custom Errors May 27, 2020
@tinovyatkin tinovyatkin changed the title DRY Custom Errors Remove code duplication in custom errors May 27, 2020
Copy link
Member

@Richienb Richienb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should keep the separate errors in their own files but importing from the base class instead.

@tinovyatkin

This comment has been minimized.

@Richienb

This comment has been minimized.

@LinusU

This comment has been minimized.

src/errors/errors.js Outdated Show resolved Hide resolved
@tinovyatkin

This comment has been minimized.

@Richienb

This comment has been minimized.

@tinovyatkin

This comment has been minimized.

@tinovyatkin tinovyatkin changed the title Remove code duplication in custom errors chore: remove code duplication in custom errors May 29, 2020
@tinovyatkin
Copy link
Member Author

@LinusU

Although, have you checked that the stack traces actually looks identical after making this change? I might be wrong here, but I think that when you do Error.captureStackTrace in the parent class, referencing this.constructor might be referencing the parent constructor, which will leave the leaf class in the stack trace?

Not, it will be referencing the actual class constructor. This is how super works.

@xxczaki xxczaki mentioned this pull request May 29, 2020
35 tasks
Copy link
Member

@xxczaki xxczaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Copy link
Member

@JefferyHus JefferyHus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tinovyatkin tinovyatkin merged commit 8f406b7 into node-fetch:master Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants