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

Addexecute #58

Merged
merged 6 commits into from Dec 6, 2017
Merged

Addexecute #58

merged 6 commits into from Dec 6, 2017

Conversation

lotkao
Copy link
Contributor

@lotkao lotkao commented Dec 3, 2017

This addresses two issues:

@lotkao lotkao closed this Dec 3, 2017
@lotkao lotkao reopened this Dec 3, 2017
@lotkao lotkao closed this Dec 3, 2017
@lotkao lotkao reopened this Dec 3, 2017
@lotkao
Copy link
Contributor Author

lotkao commented Dec 3, 2017

sorry for the opening and closing... :)

@nwwells
Copy link
Collaborator

nwwells commented Dec 4, 2017

I'm not experiencing the behavior you describe in #45 (comment) when I use master. That is to say, if I leave out --no-color, colors work properly, and if I add it, no colors are used. Moreover, --no-color seems more inline with other switches (currently just --no-check). Can you clarify your issue there?

Other than that, the execute option looks good.

@nwwells
Copy link
Collaborator

nwwells commented Dec 4, 2017

Oh, also, please clean up the whitespace to be consistent with the rest of the code. Spaces around else, if conditions, and what looks like bad indentation on L245.

@lotkao
Copy link
Contributor Author

lotkao commented Dec 4, 2017

I will clean up the code.

@lotkao
Copy link
Contributor Author

lotkao commented Dec 4, 2017

OK, I tested the master and it is working. Learned something new... I have reverted my changes regarding --no-color. I think the reason I had issues with it was because I started working on what came with the node install, and it does not have the --no-color yet... then when I forked and started working on the code here, I tried to merge my code and must have missed something that caused it to not work for me.

@lpinca
Copy link
Member

lpinca commented Dec 4, 2017

#41 is probably a cleaner approach to the same problem.

@lotkao
Copy link
Contributor Author

lotkao commented Dec 4, 2017

I tried to pipe, but in my case the connection was closed immediately, and I needed a way to send the command after the connection had been established.

Here an example using the current master:

$ echo '{"val1":1,"val2":2,"val3":3}' | wscat -c ws://[SERVER-IP]:[PORT]/url
{"val1":1,"val2":2,"val3":3}
error: Error: closed before the connection is established

Here an example using the wscat that includes the --keep-open
https://raw.githubusercontent.com/websockets/wscat/13babed39ebbdbf813a0a922c4c219b744c62075/bin/wscat

$ echo '{"val1":1,"val2":2,"val3":3}' | wscat -c ws://[SERVER-IP]:[PORT]/url --keep-open
{"val1":1,"val2":2,"val3":3}
connected (press CTRL+C to quit)
> 

In both cases the query string was echo'd before the connection was properly established.

The --keep-open is not a solution for me. The way I am using wscat, I don't want it to keep the connection open indefinitely, that's why I even added the part where it calls ws.close() (with a wait time), after sending the passed on string.

Also, when using -x or --execute, I removed the < in-front of what the server returns, so I can parse the result without having to strip the character.

@lpinca
Copy link
Member

lpinca commented Dec 4, 2017

We should probably rewrite the module, use streams and simplify everything. I like what @jnordberg did in wscat2.

I'll leave this to @nwwells.

@nwwells
Copy link
Collaborator

nwwells commented Dec 6, 2017

#41 apparently doesn't work. Not sure how it could, tbh.
#57 is similar, but without the raw output and --wait option.

I'm not wild about the interface, but we need to refactor loads of stuff anyway. For starters, using stdin should be the default. and you should close the connection on EOF, not on a timer. That being said, this PR doesn't prevent us from doing that in the future.

@nwwells nwwells merged commit 1372acd into websockets:master Dec 6, 2017
@nwwells nwwells mentioned this pull request Dec 6, 2017
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.

Blue is too dark
3 participants