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
Adding UUIDs to MessageRequests and MessageResponses #2240
Conversation
…ibrary-js into jadahiya/message-ids
…icrosoft-teams-library-js into jadahiya/message-ids
…ibrary-js into jadahiya/message-ids
…icrosoft-teams-library-js into jadahiya/message-ids
Just to let you know my philosophy as I review this:
I haven't started looking at the code yet but this is how I'm thinking about the shape of these changes. Would love to hear more on your thinking about this philosophy and how @TrevorJoelHarris feels about it as well. I don't want to start reviewing until we agree on this because the way I look at the code will vary wildly if our perspectives differ here. #Closed |
That's an interesting perspective, some of the design considerations were made to not break communication between older versions of the Hosts with newer versions of this SDK and vice versa, newer versions of the Hosts with older versions of this SDK, but I believe that can be reflected in the serialization/deserialization part of the code. I will look into incorporating some of this feedback in the meantime. |
I don't know if having an "interesting" perspective is a good or bad thing? 😅 My desire to use strongly-typed UUID object should only affect the internal implementation within teams-js and not affect back-compat since the serialized message types wouldn't be affected by the internal-to-teamsjs usage. #Closed |
Poor choice of words on my part. By interesting idea, I meant that I liked it and wanted to look into making the changes to the code to reflect it. 😅 And my second sentence was more to say that using a strongly-typed UUID object wouldn't affect back compat since we can always serialize it to a string or number (for back compat) before putting the message on the wire as you brought up above. |
Sweet and thank you for considering my input and the value of type-safety across our codebase. And won't it be fun to define a custom class to hold a UUID? And then we can start using it elsewhere in the code, too! #Closed |
…ibrary-js into jadahiya/message-ids
…ibrary-js into jadahiya/message-ids
…ommunication and id as numbers communication
cf723f1
…e and removeHandlers
…icrosoft-teams-library-js into jadahiya/message-ids
2b41e03
…ibrary-js into jadahiya/message-ids
…icrosoft-teams-library-js into jadahiya/message-ids
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Adding UUIDs to MessageRequests and MessageResponses * Committing changefile * Added in MessageID and MessageUUID types, added in testing for UUID communication and id as numbers communication * Changing waitForResponse to have input of type MessageUUID * Updated references to use MessageID and MessageUUID * Fixed lint error * Created BaseUUID type * Added validation for uuid to handleParentMessage * Only validate UUIDs if there is one * Added in MessageUUID object * Testing new changes to UUID * Added in serialization of objects * Updated to address PR feedback * Added comment describing MessageID vs MessageUUID * Added in helper function * Removing comment since map.delete was verified to work correctly * Moved uuid object to interfaces.ts for more general use * Replaced Function in Map declarations * Updated to use serialized and deserialized Message Response objects * Added in unit tests for UUID class * Fixed lint error * Updating tests * Moved uuid object to new file uuidObject.ts, edited changefile verbage * Added in unit tests for testing callback map deletion and functionality * Removed unnused import * Added in logging for when a callbackID fails to be generated * Fixed a typo in communication spec * Updated serialization and deserialization and uuid toString function * Reverted toString() change due to compatibility issues * Removing unnecessary map clear due to already clearing in uninitialize and removeHandlers * Fixed an issue with removing message ids * Reverting uuid back to private --------- Co-authored-by: Trevor Harris <trharris@microsoft.com> Co-authored-by: AE ( ͡ಠ ʖ̯ ͡ಠ) <36546967+AE-MS@users.noreply.github.com>
* Adding UUIDs to MessageRequests and MessageResponses * Committing changefile * Added in MessageID and MessageUUID types, added in testing for UUID communication and id as numbers communication * Changing waitForResponse to have input of type MessageUUID * Updated references to use MessageID and MessageUUID * Fixed lint error * Created BaseUUID type * Added validation for uuid to handleParentMessage * Only validate UUIDs if there is one * Added in MessageUUID object * Testing new changes to UUID * Added in serialization of objects * Updated to address PR feedback * Added comment describing MessageID vs MessageUUID * Added in helper function * Removing comment since map.delete was verified to work correctly * Moved uuid object to interfaces.ts for more general use * Replaced Function in Map declarations * Updated to use serialized and deserialized Message Response objects * Added in unit tests for UUID class * Fixed lint error * Updating tests * Moved uuid object to new file uuidObject.ts, edited changefile verbage * Added in unit tests for testing callback map deletion and functionality * Removed unnused import * Added in logging for when a callbackID fails to be generated * Fixed a typo in communication spec * Updated serialization and deserialization and uuid toString function * Reverted toString() change due to compatibility issues * Removing unnecessary map clear due to already clearing in uninitialize and removeHandlers * Fixed an issue with removing message ids * Reverting uuid back to private --------- Co-authored-by: Trevor Harris <trharris@microsoft.com> Co-authored-by: AE ( ͡ಠ ʖ̯ ͡ಠ) <36546967+AE-MS@users.noreply.github.com>
For more information about how to contribute to this repo, visit this page.
Description
Main changes in the PR:
uuid
parameter toMessageRequest
andMessageResponse
objectsid
to be either a string or number now.