fix: Add functionality for grpc camel case to respect splitting by word #721

iangregsondev · 2022-12-09T11:31:56Z

I am using the exact same functionality found in the grpc nodejs library.

I also added a small test.

It now converts GetAPIKey -> getApiKey

Before the change it would be -> getAPIKey

This obviously fails inside the grpc nodejs library.

I have run the tests and of course that works but I was unable to run

yarn build:test

I am on a M1 and get

 => ERROR [7/8] RUN protoc --version                                                                                                          0.2s 
------                                                                                                                                             
 > [7/8] RUN protoc --version:
#0 0.144 qemu-x86_64: Could not open '/lib64/ld-linux-x86-64.so.2': No such file or directory
------
failed to solve: executor failed running [/bin/sh -c protoc --version]: exit code: 255
error Command failed with exit code 17.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 17.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

I know that this issue was raised and fixed previously but it doesn't work for me :-(

iangregsondev · 2022-12-09T11:32:28Z

@stephenh Could you take a look for me, please? Thanks.

iangregsondev · 2022-12-09T11:37:08Z

There is a failing check :-( Not sure what that is.

iangregsondev · 2022-12-09T14:19:41Z

I noticed that lodash I was using was included in the packages/lock but not as a direct dependency, added it.

iangregsondev · 2022-12-09T14:23:39Z

Oh, I see the checks are all passing now, but before the 16.x was failing, but all seems good now.

iangregsondev · 2022-12-09T14:37:17Z

This is where the grpc library does its conversion.

https://github.com/grpc/grpc-node/blob/f457a9bfa102fc4c7a7213ac0f926346baa49036/packages/proto-loader/src/index.ts#L19

and

https://github.com/grpc/grpc-node/blob/f457a9bfa102fc4c7a7213ac0f926346baa49036/packages/proto-loader/src/index.ts#L242

stephenh · 2022-12-09T14:46:39Z

@iangregsondev I just added a .yarnrc.yml file that I think will fix the docker issue for you:

https://github.com/stephenh/ts-proto/blob/main/.yarnrc.yml

Can you try running/rebasing on main and see if that works?

package.json

src/case.ts

* chore: Bump to yarn v3. * Bump angular example.

iangregsondev · 2022-12-09T15:13:03Z

This did not work I am afraid.

iangregsondev · 2022-12-09T16:01:50Z

@stephenh Sorry had some issues :-(

Couldn't update the lodash to use the one suggested because it gives an issue with esModuleInterop, so I had to add it to the tsconfig but then I get issues in the tests.

I also updated the protoc and passing the required parameter but it kept failing :-(

I have returned it back to the way it was.

I added a comment though as suggested.

let me know your thoughts.

iangregsondev · 2022-12-09T16:05:11Z

Oh Wait, I think I updated the wrong tsconfig. Let me have another play :-)

iangregsondev · 2022-12-09T16:20:09Z

@stephenh Sorry mate, no go, it causes me issues if import lodash.camel as I have to enable esModuleInterop but all tests start to fail.

I am probably missing something :-(

Help ?

Would this be a major concern as this is a build tool anyway and keep the size down for front end use is probably not a concern here.

Let me know your feedback, maybe you see the issue ?

Failing that I could extract the lodash internal implementation :-)

Speak soon.

stephenh · 2022-12-09T16:32:15Z

Ah damn, yeah it's been awhile since I've fought with esModuleInterop but yeah it can be finicky... I dunno, I don't actually hate the idea of just copy/pasting in their camel case; assuming that it's a short/self-contained method, yeah let's try that? Otherwise yeah I'm fine with the full lodash dependency; thanks!

…

On Fri, Dec 9, 2022, 10:20 AM Ian Gregson ***@***.***> wrote: @stephenh <https://github.com/stephenh> Sorry mate, no go, it causes me issues if import lodash.camel as I have to enable esModuleInterop but all tests start to fail. I am probably missing something :-( Help ? Would this be a major concern as this is a build tool anyway and keep the size down for front end use is probably not a concern here. Let me know your feedback, maybe you see the issue ? Failing that I could extract the lodash internal implementation :-) Speak soon. — Reply to this email directly, view it on GitHub <#721 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAABSALFEYWMTSYWT2NY6FLWMNL4JANCNFSM6AAAAAASZHWZYE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

iangregsondev · 2022-12-09T17:30:59Z

The internal implementation for camelCase inside of lodash was a mess :-)

A new library found that is a native typescript library and only 1.4K in size compared to lodash which was 71k :-)

It also supports precisely what we need to do.

Also updated the comment to remove the reference to lodash as we are no longer using it.

iangregsondev · 2022-12-09T17:34:31Z

@stephenh Sometimes the build seems to fail, for example, before it failed all of them :-(

Maybe an issue of Github Actions, I don't think its anything I have done. I had this before.

Error: 14 UNAVAILABLE: Stream refused by server

Could you force a new build please, I don't think I have rights.

iangregsondev · 2022-12-09T17:43:37Z

@stephenh I updated a comment and it forced a build :-) It has now passed.

Over to you :-)

Cheers.

stephenh · 2022-12-09T17:49:32Z

This looks great! Thanks @iangregsondev !

## [1.135.1](v1.135.0...v1.135.1) (2022-12-09) ### Bug Fixes * Add functionality for grpc camel case to respect splitting by word ([#721](#721)) ([4af040c](4af040c)), closes [#722](#722)

stephenh · 2022-12-09T17:56:39Z

🎉 This PR is included in version 1.135.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Hoppingmad9 · 2023-04-14T12:41:46Z

I believe this change breaks service names that use snake_case.

With version 1.135.0 the following proto

service General
{
    rpc general_analysis_calculation(GeneralInputs) returns (GeneralOutputsList) {}
}

generates

export interface GeneralServer extends UntypedServiceImplementation {
  general_analysis_calculation: handleUnaryCall<GeneralInputs, GeneralOutputsList>;
}

export interface GeneralClient extends Client {
  general_analysis_calculation(
    request: GeneralInputs,
    callback: (error: ServiceError | null, response: GeneralOutputsList) => void,
  ): ClientUnaryCall;
  general_analysis_calculation(
    request: GeneralInputs,
    metadata: Metadata,
    callback: (error: ServiceError | null, response: GeneralOutputsList) => void,
  ): ClientUnaryCall;
  general_analysis_calculation(
    request: GeneralInputs,
    metadata: Metadata,
    options: Partial<CallOptions>,
    callback: (error: ServiceError | null, response: GeneralOutputsList) => void,
  ): ClientUnaryCall;
}

maintaining the snake_case (general_analysis_calculation).

Then with version 1.135.1 the same proto generates

export interface GeneralServer extends UntypedServiceImplementation {
  generalAnalysisCalculation: handleUnaryCall<GeneralInputs, GeneralOutputsList>;
}

export interface GeneralClient extends Client {
  generalAnalysisCalculation(
    request: GeneralInputs,
    callback: (error: ServiceError | null, response: GeneralOutputsList) => void,
  ): ClientUnaryCall;
  generalAnalysisCalculation(
    request: GeneralInputs,
    metadata: Metadata,
    callback: (error: ServiceError | null, response: GeneralOutputsList) => void,
  ): ClientUnaryCall;
  generalAnalysisCalculation(
    request: GeneralInputs,
    metadata: Metadata,
    options: Partial<CallOptions>,
    callback: (error: ServiceError | null, response: GeneralOutputsList) => void,
  ): ClientUnaryCall;
}

which has converted the name to camelCase (generalAnalysisCalculation).

I'm using the following options when generating the TypeScript.

const PROTO_DIR = path.join(__dirname, '../proto_files')
const MODEL_DIR = path.join(__dirname, '../src/models/protos')
const PROTOC_PATH = path.join(__dirname, '../node_modules/grpc-tools/bin/protoc')
const PLUGIN_PATH = path.join(__dirname, '../node_modules/.bin/protoc-gen-ts_proto')
const protoConfig = [
  `--plugin=${PLUGIN_PATH}`,

  // https://github.com/stephenh/ts-proto/blob/main/README.markdown
  '--ts_proto_opt=outputServices=grpc-js,env=node,useOptionals=messages,exportCommonSymbols=false,esModuleInterop=true,snakeToCamel=false',

  `--ts_proto_out=${MODEL_DIR}`,
  `--proto_path ${PROTO_DIR} ${PROTO_DIR}/*.proto`
]
execSync(`${PROTOC_PATH} ${protoConfig.join(' ')}`)

It's not a huge issue as a quick find/replace across the workspace fixes it, and when building the JS my compiler suggested the "new" function name as I guess snake/camel difference was easy enough to detect.

stephenh · 2023-04-15T14:29:02Z

@Hoppingmad9 FWIW from @iangregsondev 's fix in this PR, I had assumed the method names of generalAnalysisCalculation had to line up with what the grpc-js node library assumed, b/c if we got the name translation wrong (i.e. getAPIKey instead of getApiKey) things would break (i.e. that's why he opened this PR and fixed it).

But your assertion is that grpc-js works fine with general_analysis_calculation names? Did you also have to configure it with a similar "snake to camel false" config option?

Hoppingmad9 · 2023-04-17T08:53:03Z

We don't do any config for grpc-js.

So

our proto file
the C++ code the requests go to
my TypeScript code that references the ts-proto generated code

all use general_analysis_calculation.

Everything worked when ts-proto generated it as snake_case.
Then when I updated ts-proto and generated it with camelCase it broke, but I only had to rename the functions in the TypeScript code to make it all work again.
So camelCase in typescript and snake_case in the proto and C++ works, but this doesn't seem like behaviour that should be relied upon.

I'm happy to change our code/proto to use camelCase everywhere so don't worry about this for us.

Add functionality for grpc camel case to respect splitting by word

62716c8

iangregsondev changed the title ~~Add functionality for grpc camel case to respect splitting by word~~ fix: Add functionality for grpc camel case to respect splitting by word Dec 9, 2022

iangregsondev mentioned this pull request Dec 9, 2022

Lowercasing does not lowercase "API" of FindAPIKeys, for example. #698

Closed

Add lodash as a direct dependency

a041ab6

stephenh approved these changes Dec 9, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

src/case.ts Show resolved Hide resolved

stephenh and others added 3 commits December 9, 2022 15:56

chore: Bump to yarn v3. (stephenh#722)

989ecd2

* chore: Bump to yarn v3. * Bump angular example.

Rebase from upstream

b1b43b4

Update dependencies

6598193

Ian Gregson added 5 commits December 9, 2022 16:14

Depend only on lodash.camelcase to reduce import size

9d9962c

Fix merge conflicts

61e9b73

Fix an issue with esModuleInterop

6edf29b

Fix an issue with esModuleInterop

4a72629

Add full version of lodash back

ad5c6e9

Ian Gregson added 2 commits December 9, 2022 17:10

Try fixing esModuleInterop

c9fd3fc

Remove esModuleInterop

e77e371

Ian Gregson added 2 commits December 9, 2022 18:17

Replace library for using camel casing by word

7fa7c2f

Force rebuild

ec16765

Update comment

4c3fa3a

stephenh merged commit 4af040c into stephenh:main Dec 9, 2022

stephenh added the released label Dec 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Add functionality for grpc camel case to respect splitting by word #721

fix: Add functionality for grpc camel case to respect splitting by word #721

iangregsondev commented Dec 9, 2022

iangregsondev commented Dec 9, 2022

iangregsondev commented Dec 9, 2022

iangregsondev commented Dec 9, 2022

iangregsondev commented Dec 9, 2022

iangregsondev commented Dec 9, 2022

stephenh commented Dec 9, 2022

iangregsondev commented Dec 9, 2022

iangregsondev commented Dec 9, 2022

iangregsondev commented Dec 9, 2022

iangregsondev commented Dec 9, 2022

stephenh commented Dec 9, 2022 via email

iangregsondev commented Dec 9, 2022

iangregsondev commented Dec 9, 2022

iangregsondev commented Dec 9, 2022

stephenh commented Dec 9, 2022

stephenh commented Dec 9, 2022

Hoppingmad9 commented Apr 14, 2023

stephenh commented Apr 15, 2023

Hoppingmad9 commented Apr 17, 2023

fix: Add functionality for grpc camel case to respect splitting by word #721

fix: Add functionality for grpc camel case to respect splitting by word #721

Conversation

iangregsondev commented Dec 9, 2022

iangregsondev commented Dec 9, 2022

iangregsondev commented Dec 9, 2022

iangregsondev commented Dec 9, 2022

iangregsondev commented Dec 9, 2022

iangregsondev commented Dec 9, 2022

stephenh commented Dec 9, 2022

iangregsondev commented Dec 9, 2022

iangregsondev commented Dec 9, 2022

iangregsondev commented Dec 9, 2022

iangregsondev commented Dec 9, 2022

stephenh commented Dec 9, 2022 via email

iangregsondev commented Dec 9, 2022

iangregsondev commented Dec 9, 2022

iangregsondev commented Dec 9, 2022

stephenh commented Dec 9, 2022

stephenh commented Dec 9, 2022

Hoppingmad9 commented Apr 14, 2023

stephenh commented Apr 15, 2023

Hoppingmad9 commented Apr 17, 2023