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: verbose call method is broken #7

Closed
wants to merge 2 commits into from

Conversation

m-schmoock
Copy link

The verbose call feature seems broken as the Client will always raise method parameter is required. This RP creates the missing testcases that for now fail by intention.

Note: The verbose syntax is introduced on the projects main README.md and the feature is just not working

@m-schmoock m-schmoock changed the title chore: adds missing test that shows the verbose call method is broken verbose call method is broken Feb 6, 2019
@m-schmoock m-schmoock changed the title verbose call method is broken fix: verbose call method is broken Feb 6, 2019
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 91.096% when pulling 2f5c53b on m-schmoock:master into 2ccf319 on PlayNetwork:master.

@m-schmoock
Copy link
Author

Also we might have a bug in the message serializer where nodejs will send a stringified string, mening the payload will have "{...}" quotes instead of {...} which causes the server to complain. Why is the npm 1.0.3 release looking so much different to the sourcecode in this repo??

@brozeph brozeph self-requested a review February 6, 2019 19:28
@brozeph
Copy link
Contributor

brozeph commented Feb 6, 2019

On line 60 of the Client.js module, there is a ternary expression used to substitute the method argument in as the JSON-RPC request. However, the method argument must be compliant (i.e. must have a properties to defined id, method and params). Based on your findings, it looks like this ternary check is failing... I'll continue to dig and update on this PR.

@brozeph
Copy link
Contributor

brozeph commented Feb 9, 2019

Added fix in #8 - there was an error in the if statement that @m-schmoock properly detected... closing this PR in favor of #8 as Babel was updated along with Gulp to reference the latest versions.

@brozeph brozeph closed this Feb 9, 2019
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.

3 participants