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: Extend global.Error to avoid import collisions with Error proto msgs #699

Merged

Conversation

MorganIsBatman
Copy link
Contributor

We have an Error proto (I assume reasonably common) that causes typescript issues when imported into the proto file of a service definition, as the export class GrpcWebError extends Error added to the output .ts files for the service now references the imported Error type, not javascript's Error class.

Change here is to simply use global.Error to avoid the issue

…sages

We have an `Error` proto that causes typescript issues when imported into the proto file of a service definition, as the `export class GrpcWebError extends Error` added to the output .ts files for the service think we're trying to extend our type, not the js type.

Change here is to simply use `global.Error` to avoid the issue
@MorganIsBatman MorganIsBatman changed the title Extend global.Error to avoid import collisions with Error proto msgs fix: Extend global.Error to avoid import collisions with Error proto msgs Nov 9, 2022
@stephenh
Copy link
Owner

stephenh commented Nov 9, 2022

Ah yeah, @MorganIsBatman this is great! We've definitely that this issue in the past, with Error message types overlapping with Error-the-error.

Can you run yarn bin2ts to re-generate our integration suite's code with your change?

That will get the build passing + be a regression against the prefix accidentally going away in the future.

@MorganIsBatman
Copy link
Contributor Author

Oh! sorry @stephenh - I missed that step in my haste.

I've run through building the tests and running them via both the standard and :local variants on my intel Mac and M1 but both have a few tests fail for reasons I'm not totally understanding. They seem to fail when run on the master branch too?

I've committed the results of running yarn build:test:local which I see has re-generated the integration suite, but I would love any advice you may have on resolving the broken tests? 🙏

MorganIsBatman and others added 2 commits November 11, 2022 14:32
This based on feedback from someone in my team using Vite and importing the .ts file rather than the compiled .js file - using `global` errors as its declared in the same file
@stephenh
Copy link
Owner

Ah yeah, @MorganIsBatman didn't have a chance to look in to this now, but sorry, the issue was an admittedly non-obvious foot gun that the :local-suffixed targets were not really meant as "run these version locally" but more like "our legacy non-docker build setup".

The non-/pre-docker setup lead to build output flakiness b/c each contributors version of protoc would have slightly different output (even for very trivial things like the comments of the core protobuf types being updated across protoc version).

I went ahead and pushed the yarn bin2ts/docker-based output to this PR and it all looks great, so I'll go ahead and merge.

I'm also going to make a follow up PR that removes the :local targets; we'd kept them with the idea that the docker setup was new/wip, but they've outlived their usefulness at this point.

Thanks!

@stephenh stephenh merged commit e9d8f91 into stephenh:main Nov 13, 2022
stephenh pushed a commit that referenced this pull request Nov 13, 2022
## [1.131.1](v1.131.0...v1.131.1) (2022-11-13)

### Bug Fixes

* Extend `global.Error` to avoid import collisions with Error proto msgs ([#699](#699)) ([e9d8f91](e9d8f91))
@stephenh
Copy link
Owner

🎉 This PR is included in version 1.131.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@MorganIsBatman
Copy link
Contributor Author

You're awesome! Thanks for the quick turnaround and for taking care of that last step for me @stephenh

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.

2 participants