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

Remove unused function from generated code #29

Closed
wants to merge 1 commit into from
Closed

Remove unused function from generated code #29

wants to merge 1 commit into from

Conversation

remko
Copy link
Contributor

@remko remko commented Jan 2, 2022

This commit removes a function in the generated code that is not used.

The generated b64decode is currently technically also unused, and causes unnecessary bloat of the generated code. I did not remove it, because it is currently used by the test to decode the bytes result from the response (as bytes are currently left untouched as it comes back from the gateway). Maybe it was left in as a convenience for users, but this plugin probably shouldn't act as a utility package for generic operations such as base64 decoding.

I think one of the following should happen:

  • The plugin should also decode the base64 as it comes back from the gateway (to be consistent with the encoding when sending). In this case, it makes sense to keep the b64decode in, as it will be used by the generated code
  • The b64decode function should be removed, and clients should choose to include their own base64 decoding if they need it.

The former option breaks backwards compatibility, but is related to the open question on how to
fix Issue 25 : should this plugin do transformations on the data or not? The path chosen for that issue impacts the right way to handle bytes data from the server as well.

@lyonlai
Copy link
Collaborator

lyonlai commented Jan 18, 2022

few comments:

The plugin should also decode the base64 as it comes back from the gateway (to be consistent with the encoding when sending). In this case, it makes sense to keep the b64decode in, as it will be used by the generated code

It should, there were plans to do so but haven't really got the space to do it yet.

The b64decode function should be removed, and clients should choose to include their own base64 decoding if they need it.

It's generated only for test but yet to be used in production code. This is not ideal. in terms of causes unnecessary bloat of the generated code, I don't think so. Mainly because

  1. it lives in the fetch.pb.ts file which will only generate once for one protoc command invocation
  2. modern JS bundling nowadays have dead code detection and tree shaking so it wouldn't really leak into your production code if you configured properly.

And this PR only removes it but if you refresh the generated code the test will fail. A better way for now is moving the functionality to a test utility file and let the test refer to it.

@remko

@remko remko closed this by deleting the head repository May 1, 2024
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.

2 participants