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

GPII-2914: Port Binding grade now sends and waits for receipts from posted messages. #14

Merged
merged 28 commits into from Oct 15, 2018

Conversation

Projects
None yet
2 participants
@jobara
Copy link
Collaborator

jobara commented Sep 6, 2018

The port binding grade has been updated to facilitate the sending and receiving of receipts. After handling an incoming message a receipt is returned. This is particularly useful for the prefs editor's store which will wait till the writing has been handled before continuing.

https://issues.gpii.net/browse/GPII-2914

Includes changes from: #11

Needs to be updated with the latest Infusion after fluid-project/infusion#933 has been merged. This PR includes updates to work with that version of Infusion.

jobara added some commits Aug 16, 2018

GPII-2914: Refactored port connections
- using portBinding throughout
- messages passed with a type and payload
GPII-2914: writes will wait for a returned receipt.
Implemented a mechanism for sending and receiving receipts of messages 
across chrome's message passing system.

The tests have been updated.
@jobara

This comment has been minimized.

Copy link
Collaborator Author

jobara commented Sep 6, 2018

@amb26 could you please take a look at this PR.

@jobara

This comment has been minimized.

Copy link
Collaborator Author

jobara commented Sep 24, 2018

@amb26 I've updated this PR with the latest Infusion which includes the changes for https://issues.fluidproject.org/browse/FLUID-6257. Ready for review.

@@ -69,14 +288,30 @@
type: "fluid.dataSource.encoding.model"
}
},
members: {
lastIncomingPayload: {}

This comment has been minimized.

Copy link
@amb26

amb26 Sep 26, 2018

Member

I think that this particular element of the dataStore isn't useful for general clients and should be moved into the testing grade

This comment has been minimized.

Copy link
@jobara

jobara Sep 26, 2018

Author Collaborator

It's actually used as part of the get mechanism in the "onRead.impl" to return the value. Essentially this is an in memory store of the latest updates sent via the messaging system.

This comment has been minimized.

Copy link
@amb26

amb26 Sep 26, 2018

Member

Seems suspicious - shouldn't it be that the store actually issues reads in order to honour requests? In which case the read implementation should just consist of a promise which resolves at the point the appropriate payload response is received. If people want to perform caching based on such a DataSource that should be implemented at the DataSource level rather than intruding on the messaging level. Do we have a requirement for non-live reads?

This comment has been minimized.

Copy link
@jobara

jobara Sep 26, 2018

Author Collaborator

Spoke to @amb26 in the fluid-work channel about this.

In summary it is suggested to remove the "store" and use the DataSource API with the gpii.chrome.portBinding to directly get/set model values.

see: https://botbot.me/freenode/fluid-work/2018-09-26/?msg=104841751&page=2

This comment has been minimized.

Copy link
@jobara

jobara Sep 26, 2018

Author Collaborator

In a continuation of the above conversation it seems that we'll need to update the portbinding to handle more posts and messages. That is read + read-receipt and write + write-receipt.

fluid.defaults("gpii.chrome.portBinding", {
gradeNames: ["fluid.component"],
connectionName: "",
messageType: "",

This comment has been minimized.

Copy link
@amb26

amb26 Sep 26, 2018

Member

Document what this member is and how it is used

To prevent handling receipts and messages that are broadcast but not intended for a connection, a whitelist
must be provided to allow `types` of requests to be handled.
*/

fluid.defaults("gpii.chrome.portBinding", {
gradeNames: ["fluid.component"],
connectionName: "",

This comment has been minimized.

Copy link
@amb26

amb26 Sep 26, 2018

Member

Similarly explain the purpose of connectionName

openRequests: {}
},
filters: {
messages: [],

This comment has been minimized.

Copy link
@amb26

amb26 Sep 26, 2018

Member

I think it is reasonable for the component to come with some out of the box defaults for messageType, messages, receipts, etc. together with good documentation about how all of these interact

@jobara

This comment has been minimized.

Copy link
Collaborator Author

jobara commented Oct 3, 2018

@amb26 this is ready for more review.

*
* @param {String} connectionName - the name of the connection. Will be used as the prefix for the name passed
* to the connection.
* @param {String} id - the id of the connection. Will be used as the suffix for the name passed to the conneciton.

This comment has been minimized.

Copy link
@amb26

amb26 Oct 3, 2018

Member

Typo conneciton. I don't find the documentation for these two members very clear, since they use the word "connection" in two different senses. Perhaps the overall name "connectionName" isn't well chosen? I still think you could do more to explain what this name means. I think if you are referring to simply the "Port" object from the runtime API you should continue to use that term to avoid confusion, and then perhaps free up the name "connection" to refer to an abstraction which you layer on top of it.

fluid.registerNamespace("gpii.chrome.portBinding");

gpii.chrome.portBinding.type = {
READ: 0,

This comment has been minimized.

Copy link
@amb26

amb26 Oct 3, 2018

Member

I don't think these numeric constants help too much. They will be a hazard to debugging as people try to figure out what the message type means that they see in their structure, and are an open invitation to unreadable gimmicks such as "type++" : P Please return to the use of plain strings to encode the message types

This comment has been minimized.

Copy link
@amb26

amb26 Oct 3, 2018

Member

The numbers also prevent the easy coexistence of this message bus with some other open system of typed messages going over the same port

* @param {Object} data - the data to handle from the incoming port message
*/
gpii.chrome.portBinding.handleIncoming = function (that, data) {
if (data.type === gpii.chrome.portBinding.type.READ) {

This comment has been minimized.

Copy link
@amb26

amb26 Oct 3, 2018

Member

This could be a nice lookup, perhaps even stored in the component's options

* @param {Component} that - the component itself
* @param {Number} type - identifies the type of message for listeners on the other end.
* See: gpii.chrome.portBinding.type
* @param {Function} handleFn - a function to handle the message

This comment has been minimized.

Copy link
@amb26

amb26 Oct 3, 2018

Member

Explain signature here, perhaps with a @typedef

* integrator. The result or promise returned by `handleMessageImpl` is used to determine if a receipt should be
* sent with or without an error and with what payload.
*
* @param {Component} that - the component itself

This comment has been minimized.

Copy link
@amb26

amb26 Oct 3, 2018

Member

Which one?

@jobara

This comment has been minimized.

Copy link
Collaborator Author

jobara commented Oct 4, 2018

@amb26 I believe I've addressed all of your comments. Ready for more review.

"": {
namespace: "test",
listener: function (log) {
console.log("****************", log);

This comment has been minimized.

Copy link
@amb26

amb26 Oct 11, 2018

Member

Stray console.log

// defines which event handles which a message type
// message types that aren't defined here are ignored.
messageForwardingMap: {
"UIO_PLUS_READ_REQUEST": "onIncomingRead",

This comment has been minimized.

Copy link
@amb26

amb26 Oct 11, 2018

Member

The capitals and underscores convention looks odd compared to others that we use - and also it differs oddly from the namespacing of the component itself. Perhaps something like gpii.chrome.readRequest for the names?
I know that we use capital letters in the ChangeApplier messages but this is a decision that has been widely criticised and continues only through inertia...

This comment has been minimized.

Copy link
@jobara

jobara Oct 11, 2018

Author Collaborator

@amb26 won't the "." in the name cause issues for the fluid.get call that is used to lookup the mapping?

This comment has been minimized.

Copy link
@amb26

amb26 Oct 11, 2018

Member

I don't think there's a need for a fluid.get call since the mapping must always be there

},
"onIncomingRead.handle": {
listener: "{that}.handleMessage",
args: ["UIO_PLUS_READ_RECEIPT", "{that}.handleRead", "{arguments}.0"]

This comment has been minimized.

Copy link
@amb26

amb26 Oct 11, 2018

Member

As it stands an adapter of this component would have to override most of the implementation if they wanted to, e.g. share a port bus with another target. What I suggest is that
i) Within an expander you compute the inverse map of messageForwardingMap, and then
ii) Whenever you need to refer to an event name in the implementation, you refer to it via indirection on its corresponding event name - that is, these args would read "onIncomingRead"
and then in the implementation you would look up the corresponding port message type via that.options.inverseMessageMap[eventName]

This comment has been minimized.

Copy link
@jobara

jobara Oct 11, 2018

Author Collaborator

@amb26 this isn't the inbound message type, but rather the outbound one. If you still think a map is necessary, it would probably need to be one between incoming and outgoing message types. Not sure what the exact structure would look like though.. {onIncomingRead: "UIO_PLUS_READ_RECEIPT"} or {read: "UIO_PLUS_READ_RECEIPT"} or something else. I think either way those would be staticly defined in the options.. Please let me know what you think.

This comment has been minimized.

Copy link
@amb26

amb26 Oct 11, 2018

Member

Well whichever message it is you should refer to it via a stable name, and the one you have to hand is the one defined in the messageForwardingMap

@jobara

This comment has been minimized.

Copy link
Collaborator Author

jobara commented Oct 15, 2018

@amb26 I think I've addressed all of the comments. Ready for more review.

@amb26 amb26 merged commit 35f1e4b into GPII:master Oct 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.