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

Fixed: prevent codegen'ing a function with an invalid name. #870

Closed
wants to merge 1 commit into from
Closed

Fixed: prevent codegen'ing a function with an invalid name. #870

wants to merge 1 commit into from

Conversation

eventualbuddha
Copy link
Contributor

This can happen when, for example, an RPC method name is the uppercase of a keyword like "Delete". I tried to retain the performance of the original, but since the test just uses the same names over and over again and they're memoized it's not a super representative set of test data to really understand how the performance changed.

This can happen when, for example, an RPC method name is the uppercase of a keyword like "Delete".
@eventualbuddha
Copy link
Contributor Author

Ping.

@eventualbuddha
Copy link
Contributor Author

Is this project unmaintained? Do I need to do something before this can be considered?

@dcodeIO
Copy link
Member

dcodeIO commented Nov 24, 2017

Thanks, but this doesn't fix the underlying issue since these function might still be called by other parts of the library, using their original name. Instead, we should make sure that invalid names are handled on the reflection level.

@dcodeIO
Copy link
Member

dcodeIO commented Nov 27, 2017

The commit above should prevent the parse error in runtime code while still naming the method delete, hence to be accessed through myService["delete"](...). Should also be consistent with statically generated code doing the same.

@eventualbuddha
Copy link
Contributor Author

Cool, that seems like a good approach. Thanks for looking at this!

@eventualbuddha
Copy link
Contributor Author

@dcodeIO so is your solution finished? Anything I can do to help?

@dcodeIO
Copy link
Member

dcodeIO commented Nov 30, 2017

The changes are live, latest on npm is 6.8.3. Have you tested with this one?

@eventualbuddha
Copy link
Contributor Author

Oh cool, I’ll try it out tomorrow. I had assumed that since the issue was still open that the problem remained.

@dcodeIO
Copy link
Member

dcodeIO commented Feb 6, 2018

Closing for now, feel free to report any remaining issues.

@dcodeIO dcodeIO closed this Feb 6, 2018
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