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

Define 'type' as a property on errors rather than a getter #719

Merged
merged 1 commit into from
Oct 30, 2019

Conversation

richardm-stripe
Copy link
Contributor

cc @rattrayalex-stripe for discussion:

I don't have a full understanding of why it was originally important for 'type' to be implemented as a getter -- or if we should do this -- but this PR both

  1. Passes the existing test/Error.spec.js
  2. Fulfills the ask in StripeError.type missing from logs #718
      class Foo extends Error.StripeError {}
      const err = new Foo({message: 'hi'});
      console.log(err);

outputs

...
  {
  raw: { message: 'hi' },
  rawType: undefined,
  code: undefined,
  param: undefined,
  detail: undefined,
  headers: undefined,
  requestId: undefined,
  statusCode: undefined,
  charge: undefined,
  decline_code: undefined,
  payment_intent: undefined,
  payment_method: undefined,
  setup_intent: undefined,
  source: undefined,
  type: 'Foo'
}

and

      const Custom = Error.StripeError.extend({type: 'Foo'});
      const err = new Custom({message: 'hi'});
      console.log(err);

outputs the same.

Copy link
Contributor

@rattrayalex-stripe rattrayalex-stripe left a comment

Choose a reason for hiding this comment

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

Nice, yep, this looks cleaner! And yes, type should definitely be enumerable; oops.

Thanks Richard!

@richardm-stripe richardm-stripe merged commit 748411e into master Oct 30, 2019
@richardm-stripe richardm-stripe deleted the richardm-type-property branch October 30, 2019 16:56
nsallerin added a commit to fewlines-education/stripe-demo that referenced this pull request Dec 16, 2019
…ode-dependencies

## Description

**react** and **react-dom** updated from **16.9.0 to 16.12.0**

###### React DOM

* Remove `unstable_createRoot` and `unstable_createSyncRoot` experimental APIs. (These are available in the Experimental channel as `createRoot` and `createSyncRoot`.) ([@acdlite]
* Clear additional fiber fields during unmount to save memory. ([@trueadm](http://github.com/trueadm) in [#16807](facebook/react#16807))
* Prefer `Object.is` instead of inline polyfill, when available. ([@ku8ar](http://github.com/ku8ar) in [#16212](facebook/react#16212))

**react-scripts** updated from **16.9.0 to 16.12.0**

https://github.com/facebook/create-react-app/blob/master/CHANGELOG.md

**dotenv** updated from **8.1.0 to 8.2.0**

https://github.com/motdotla/dotenv/blob/master/CHANGELOG.md
---> no changes in changelog

**stripe** updated from **7.9.1 to 7.14.0**

- [#699](stripe/stripe-node#699) Add request-specific fields from raw error to top level error
- [#719](stripe/stripe-node#719) Define 'type' as a property on errors rather than a getter
- [#728](stripe/stripe-node#728) Remove duplicate export

## Related Issue

[ch1243]

## Motivation and Context

We want our dependencies up to date

## How Has This Been Tested?

Running `yarn test` all tests passed

## Types of changes

- Chore (non-breaking change which refactors / improves the existing code base)
- ~Bug fix (non-breaking change which fixes an issue)~
- ~New feature (non-breaking change which adds functionality)~
- ~Breaking change (fix or feature that would cause existing functionality to 
  change)~

## Checklist:

- ✅ My code follows the code style of this project.
- 🔴 My change requires a change to the documentation.
- 🔴 I have updated the documentation accordingly.
- ✅ I have read the [**CONTRIBUTING**][CONTRIBUTING_FILE] document.
- 🔴 I have added tests to cover my changes.
- ✅ All new and existing tests passed.

[CONTRIBUTING_FILE]: https://github.com/fewlinesco/connect/blob/master/CONTRIBUTING.md
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.

3 participants