feat: sends modal event data to modal wrapper for use in hooks #1170
Conversation
| export class PostMessengerMessage { | ||
| constructor(type, eventName, eventPayload) { | ||
| this.eventName = eventName; | ||
| this.id = createUUID(); | ||
|
|
||
| if (type === 'ack') { | ||
| this.type = POSTMESSENGER_EVENT_TYPES.ACK; | ||
| this.eventPayload = POSTMESSENGER_ACK_PAYLOAD; | ||
| } else if (type === 'message') { | ||
| this.type = POSTMESSENGER_EVENT_TYPES.MESSAGE; | ||
| this.eventPayload = eventPayload; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Does this need to be a class or could it just be a createMessage(type, eventName, eventPayload) that returns a simple object? Using a class here without any methods that just holds data seems like may be reaching for a more complex data structure.
If we include sendEvent as a method on this class then it makes more sense. Maybe this could look like
const event = new PostMessengerEvent(/* ... */);
event.sendTo(clientOrigin);There was a problem hiding this comment.
I appreciate the suggestion building off my idea to make it an object. Considered adding a method to that class, then realized we would get a DataCloneError due to postMessage trying to clone the object and it having a function. So I think your suggestion we toss the class in favor of a function returning a simple object is the way to go.
| function createUUID() { | ||
| // crypto.randomUUID() is only available in HTTPS secure environments and modern browsers | ||
| if (typeof crypto !== 'undefined' && crypto && crypto.randomUUID instanceof Function) { | ||
| return crypto.randomUUID(); | ||
| } | ||
|
|
||
| const validChars = '0123456789abcdefghijklmnopqrstuvwxyz'; | ||
| const stringLength = 32; | ||
| let randomId = ''; | ||
| for (let index = 0; index < stringLength; index++) { | ||
| const randomIndex = Math.floor(Math.random() * validChars.length); | ||
| randomId += validChars.charAt(randomIndex); | ||
| } | ||
| return randomId; | ||
| } |
There was a problem hiding this comment.
Left a similar comment on a previous PR. Is there an existing util in belter that serves the same purpose that we could leverage?
There was a problem hiding this comment.
See #1161 for response
| export const POSTMESSENGER_EVENT_NAMES = { | ||
| CALCULATE: 'paypal-messages-modal-calculate', | ||
| CLOSE: 'paypal-messages-modal-close', | ||
| SHOW: 'paypal-messages-modal-show' | ||
| }; |
There was a problem hiding this comment.
Should this be moved into the postMessage file to keep it all together?
| return randomId; | ||
| } | ||
|
|
||
| export function sendEvent(payload, trustedOrigin) { |
There was a problem hiding this comment.
The trustedOrigin originates from props.origin which won't exist for all integration (e.g. lander pages). Should we have some quick short-circuit logic that early returns if the origin is undefined or empty?
| // send event ack so PostMessenger will stop reposting event | ||
| sendEventAck(id, clientOrigin); | ||
| // send event ack with original event id so PostMessenger will stop reposting event | ||
| sendEvent(new PostMessengerMessage('ack', id)); |
There was a problem hiding this comment.
Is this missing the clientOrigin as the second argument?
There was a problem hiding this comment.
Oop, will fix
| this.eventPayload = POSTMESSENGER_ACK_PAYLOAD; | ||
| } else if (type === 'message') { | ||
| this.type = POSTMESSENGER_EVENT_TYPES.MESSAGE; | ||
| this.eventPayload = eventPayload; |
There was a problem hiding this comment.
I'd like to be extra cautious here. This is the data that we are exposing publicly for anyone to access. Right now nothing sensitive is being exposed, but in the future that could accidentally change. Can we add an extra layer of validation here by only allowing explicitly approved fields to be included? Something like createSafePayload(eventPayload) that returns an object that only includes allow listed fields managed by the createSafePayload function. We should also add an explicit comment that explains the context and reason for this as it may seem redundant.
| @@ -115,10 +129,10 @@ const setupBrowser = props => { | |||
| }); | |||
| }, | |||
| onClose: ({ linkName }) => { | |||
| if (isIframe && document.referrer) { | |||
| const targetOrigin = new window.URL(document.referrer).origin; | |||
There was a problem hiding this comment.
Removing this will break some existing integrations as this value is used by integrations not passing in the props.origin manually today. Does this by chance work in place of the manual origin query param we introduced for our needs to simplify the new integration?`
There was a problem hiding this comment.
Oh good catch! I can make the document.referrer's origin a fallback in the case the origin isn't passed manually so both cases are covered
…ub.com/paypal/paypal-messaging-components into feature/modal-wrapper-events
Description
Sends messages to browser modal wrapper on modal events onShow, onClose, and onCalculate.
Screenshots
Testing instructions
See corresponding PR 637 in core-web-sdk for testing instructions.