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

feat(Grpc-Web): Add & export GrpcWebError type #593

Merged
merged 3 commits into from
Aug 15, 2022

Conversation

zakhenry
Copy link
Contributor

This allows clients to declare the error type in an observable response, e.g.

const client = new DashStateClientImpl(rpc);
client.UserSettings({}).pipe(
  catchError((err: GrpcWebError) => {
    
    if (err.code === grpc.Code.FailedPrecondition) {
      // handle appropriately
    }
    
    return throwError(() => err)
    
  }),
);

src/main.ts Outdated
const GrpcWebError = conditionalOutput(
'GrpcWebError',
code`
export interface GrpcWebError extends Error {
Copy link
Owner

Choose a reason for hiding this comment

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

@zakhenry sorry for the late review here, this looks great!

Just curious, but wdyt about making this a class, like:

export class GrpcWebError extends Error {
  constructor(message: string, public code: grpc.Code, public metadata: grpc.Metadata) {
    super(message);
  }
}

As this would let callers do things like if error instanceof GrpcWebError.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep I like it, commit pushed to implement this idea.

export class GrpcWebError extends Error {
constructor(message: string, public code: grpc.Code, public metadata: grpc.Metadata) {
super(message);
Object.setPrototypeOf(this, GrpcWebError.prototype);
Copy link
Owner

Choose a reason for hiding this comment

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

@zakhenry what's the setPrototypeOf for? I've not seen it in constructors before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stephenh it is a recommendation for extending built-in classes to ensure instanceof checks still work. See https://github.com/Microsoft/TypeScript-wiki/blob/main/Breaking-Changes.md#extending-built-ins-like-error-array-and-map-may-no-longer-work where it first appeared.

I'm unsure if this is only an issue for certain compilation targets or not, however I do have a memory from the past where not having this line has caused problems

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reading around a bit I believe this is specifically for ES5 targets, see also https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error

Copy link
Owner

Choose a reason for hiding this comment

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

Ah huh! Okay... I guess one more naive question, but this makes it sound like the es2015 target doesn't have this issue:

https://www.dannyguo.com/blog/how-to-fix-instanceof-not-working-for-custom-errors-in-typescript/#upgrade-the-compilation-target

And given it's ~7 years after 2015 :-D do you think we could just drop this as probably not necessary anymore? Like is this specifically required for your app/target's target environment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

me personally nope, I'm targeting es2015 for my applications. I'll check what is the default support for things like create-react-app and the angular cli. If they support es5 out of the box as default I think grpc-web probably should too?

Copy link
Contributor Author

@zakhenry zakhenry Aug 15, 2022

Choose a reason for hiding this comment

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

ok yep I create a fresh react app with typescript and tried it out and the instanceof check works fine. And Angular targets ES2017 by default. Happy to drop it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed.

…pcWebError.prototype);` as ES5 is deprecated
@stephenh
Copy link
Owner

Looks great, thanks @zakhenry !

@stephenh stephenh merged commit 645987d into stephenh:main Aug 15, 2022
stephenh pushed a commit that referenced this pull request Aug 15, 2022
# [1.122.0](v1.121.6...v1.122.0) (2022-08-15)

### Features

* **Grpc-Web:** Add & export GrpcWebError type ([#593](#593)) ([645987d](645987d))
@stephenh
Copy link
Owner

🎉 This PR is included in version 1.122.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

2 participants