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(data): make DataServiceError extend from Error #3988

Merged
merged 1 commit into from
Aug 5, 2023
Merged

fix(data): make DataServiceError extend from Error #3988

merged 1 commit into from
Aug 5, 2023

Conversation

rob4226
Copy link
Contributor

@rob4226 rob4226 commented Aug 5, 2023

I was kind of surprised when DataServiceError instanceof Error returned false but then I saw the note in the source code (4 years ago) about how dse instanceof DataServiceError didn't work in unit tests when extending from Error. As long as super() is called in the child class constructor when extending from Error, then instanceof should work fine. I made the change, then ran the unit tests and they all passed so it seems it's ok.

This is not a big deal but it's usually good form to have Error in the inheritance chain of custom errors. I don't believe this is a breaking change.

Makes the DataServiceError class inherit from the builtin JavaScript Error.
@netlify
Copy link

netlify bot commented Aug 5, 2023

Deploy Preview for ngrx-io canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 29f51c4
🔍 Latest deploy log https://app.netlify.com/sites/ngrx-io/deploys/64ce17d894c86b0008fb8801

Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

Weird, I wonder why this failed before.
It seems to work now 👍

@brandonroberts brandonroberts merged commit 0b98a65 into ngrx:main Aug 5, 2023
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