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

Arbitrary event argument type in CLI #134

Closed
aqrln opened this issue May 5, 2017 · 5 comments
Closed

Arbitrary event argument type in CLI #134

aqrln opened this issue May 5, 2017 · 5 comments

Comments

@aqrln
Copy link
Member

aqrln commented May 5, 2017

The JSTP CLI always passes the arguments of the event command as an array to the Connection#emitRemoteEvent method, while it should be an arbitrary JavaScript value. If user wants it to be an array, they must explicitly pass an array. In fact, this argument is an object in most cases.

This behavior may change in the future when we have metadata, so the approach implemented in the CLI may be not as incorrect as too forward-looking, but for now it is both inconsistent with the specification and making it impossible to use in the real cases that we have in our apps.

Ref: #132 (comment)

@aqrln aqrln changed the title Event arbitrary event argument type in CLI Arbitrary event argument type in CLI May 5, 2017
@belochub
Copy link
Member

belochub commented May 6, 2017

Actually, it is more of a workaround for the fact, that JSTP in its current implementation does not allow to seamlessly pass multiple arguments when emitting the event (like in EventEmitter's emit).
Considering the fact that I was using this library that way, because I didn't really understand the fact that args should be an arbitrary value and not always an array (like in callMethod method), it is probably a good idea to actually change API to allow emitRemoteEvent being called with arbitrary amount of arguments and then sending it as an array over the network. This is a breaking change, but it actually clarifies usage of this method, making it more similar to EventEmitter's emit.
Probably that's the way it will be implemented with metadata, but in my opinion it's better to have this even before metadata will be implemented.

Also, current approach implemented in the CLI is not really inconsistent with the specification, here is emitRemoteEvent's description:

// Send an event packet over the connection
// interfaceName - name of an interface
// eventName - name of an event
// args - event arguments as an object

As you can see, args is supposed to be an object, not any arbitrary value and array is an object in JavaScript, which means that current CLI implementation is just a possible interpretation of the specification.

So, to make implementation compliant with the current specification, we should allow user to provide only one array or one object as argument when sending an event. But to make our CLI more user-friendly I propose to allow users to drop square braces when they want to send an array by default, allowing for call-like syntax.
What do you think, @lundibundi?

@lundibundi
Copy link
Member

  • completely agree with first part - it'll not only be similar to EventEmitter but also to call (in my opinion if there must be only argument in emitRemoteEvent it should've been named arg)
  • about spec - that's just playing with words, it obviously means highest level of object abstraction, so args can be either array or other object, so it is a bug

I think we should change jstp according to the first statement and make square braces optional in event cli syntax.

@lundibundi
Copy link
Member

@aqrln ping.

@aqrln
Copy link
Member Author

aqrln commented May 11, 2017

@lundibundi pong. What @belochub said sounds reasonable to me, so I'm in favor of changing this.

@aqrln
Copy link
Member Author

aqrln commented May 11, 2017

@belochub @lundibundi I've created a new issue for the alternative approach, but let's keep this one open until a PR that resolves either of them lands. My motivation is that whichever side we take and whether we treat this as a bug in the CLI or as a bad design decision in the protocol that the CLI implementers were not aware of, at this point we have the CLI and the remote events facilities in the library itself that don't understand each other.

lundibundi added a commit that referenced this issue May 28, 2017
Changes event message signature to always send an array
of arbitrary arguments. This makes it work same way as
EventEmitter does and also makes its usage similar to
call action.

Fixes: #134
Fixes: #162
belochub pushed a commit that referenced this issue Jan 22, 2018
Changes event message signature to always send an array
of arbitrary arguments. This makes it work same way as
EventEmitter does and also makes its usage similar to
call action.

PR-URL: #187
Fixes: #134
Fixes: #162
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>
belochub pushed a commit that referenced this issue Jan 22, 2018
Changes event message signature to always send an array
of arbitrary arguments. This makes it work same way as
EventEmitter does and also makes its usage similar to
call action.

PR-URL: #187
Fixes: #134
Fixes: #162
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>
belochub pushed a commit to metarhia/mdsf that referenced this issue Jul 19, 2018
Changes event message signature to always send an array
of arbitrary arguments. This makes it work same way as
EventEmitter does and also makes its usage similar to
call action.

PR-URL: metarhia/jstp#187
Fixes: metarhia/jstp#134
Fixes: metarhia/jstp#162
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>
belochub pushed a commit to metarhia/mdsf that referenced this issue Jul 19, 2018
Changes event message signature to always send an array
of arbitrary arguments. This makes it work same way as
EventEmitter does and also makes its usage similar to
call action.

PR-URL: metarhia/jstp#187
Fixes: metarhia/jstp#134
Fixes: metarhia/jstp#162
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants