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

Add methodName to rpc methods to allow to determine methods after code minification #857

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Szpadel
Copy link

@Szpadel Szpadel commented Jul 5, 2017

In current state when static code is minified, generated method names are replaced by single letter names.
This make almost impossible to implement correctly transport layer for services.
Those changes adds new property to methods: methodName, that contains string to original method name.
After this change you should determine called method using methodName instead of name, but old behavior is still preserved.

Also this commit create new exported type: RpcServiceMethod to allow to access new property in type safe way.

Runtime generated code was also extended with generated code.
Although methodName is appended in every runtime rpc call - I didn't found a way how to make it one time action in method generation.

Test case was also extended to make sure that methodName will containe the same value that name.

Bonus:
I fixed Travis build issue for you :-) I don't like to have that red X in my PR ;-)

@Szpadel Szpadel force-pushed the master branch 7 times, most recently from 484e36a to e9bc55b Compare July 6, 2017 09:31
@Szpadel
Copy link
Author

Szpadel commented Jul 11, 2017

@dcodeIO any chance for merge?

Szpadel and others added 2 commits July 25, 2017 16:28
Messages with optional fields will now implement their corresponding
interfaces (IMessage)

fixes protobufjs#834
fixes protobufjs#837
@@ -78,15 +78,15 @@ function performRequestOverTransportChannel(requestData, callback) {
// Listen for events:

greeter.on("data", function(response, method) {
console.log("data in " + method.name + ":", response.message);
console.log("data in " + method.nameName + ":", response.message);

Choose a reason for hiding this comment

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

nameName?

@dcodeIO
Copy link
Member

dcodeIO commented Nov 26, 2017

Can't you just do:

function rpcImpl(method, ...) {
  if (method == MyService.prototype.myMethod) { ... }
}

@Szpadel
Copy link
Author

Szpadel commented Nov 26, 2017

@dcodeIO then you would need rpc transport layer to be dependent on all services.
and you would need implement handling of those messages manually for every service, so at this point it is easier to use pure protobuf serialization and implement all rpc manually.

@MHDante
Copy link

MHDante commented Mar 23, 2018

@dcodeIO This is quite cumbersome when building a transport that works with any proto file. Please consider merging this pr.

dcodeIO added a commit that referenced this pull request May 18, 2018
…ice method names when generating static code, see #857
@dcodeIO
Copy link
Member

dcodeIO commented May 18, 2018

Does this solve it? Idea is that defining the name property explicitly should survive minification.

@avakhrenev
Copy link

@dcodeIO how to get a full method name (in format of ${serviceName}/${methodName} ) at runtime? The name property gives access to the method name only, the service name is still missing.

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.

5 participants