-
Notifications
You must be signed in to change notification settings - Fork 337
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
Conversation
0f8d47c
to
9b21517
Compare
9b21517
to
e8aeb65
Compare
e8aeb65
to
fd0076c
Compare
src/main.ts
Outdated
const GrpcWebError = conditionalOutput( | ||
'GrpcWebError', | ||
code` | ||
export interface GrpcWebError extends Error { |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Looks great, thanks @zakhenry ! |
# [1.122.0](v1.121.6...v1.122.0) (2022-08-15) ### Features * **Grpc-Web:** Add & export GrpcWebError type ([#593](#593)) ([645987d](645987d))
🎉 This PR is included in version 1.122.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
It appears there is an issue when multiple services are included within a single proto package. This results in the GrpcWebError being exported multiple times (once for each service). I am curious why common elements like GrpcWebError are not placed in a separate, dependency-free library that could be imported as needed. I understand you might prefer to avoid this approach but it would solve a lot of pains when you need to export as a library the generation. This issue leads to the following error: has already exported a member named 'GrpcWebError'. Consider explicitly re-exporting to resolve the ambiguity. |
Hi @alexisvisco ; I've avoided a separate library just because I like the simplicity of the generated code not having any runtime dependencies; once there are runtime dependencies, you need to worry about version mismatches & things like that. And it's another library to publish to npmjs/etc. 🤷 Have you seen the We could probably try putting the (ts-poet's conditionalOutput feature basically does this, but iirc only within a single file, and not across file boundaries.) |
This allows clients to declare the error type in an observable response, e.g.