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

feat: stronger postMessage handling in intents #159

Merged
merged 6 commits into from
May 17, 2017

Conversation

gregorylegarec
Copy link
Contributor

@gregorylegarec gregorylegarec commented May 17, 2017

Interesting case in onboarding, with four services, all able of starting an intent. When one of the intents was not properly ended or cancelled (when the modal is closed by clicking on its overlay for example), the listener over postMessage events was still listening for intent:ready message and still responding with its given data (here: the konnector slug).

Consquence : if an intent to EDF konnector was open and "closed" by a click on modal overlay, all future intents were invariably serving the EDF konnector, because of the old listerner always present.

To solve this issue, my first thought was to listen the iframe itself and remove the listener when it was removed from DOM, but security/permission issues occured. So I made two things :

  • The service part now listen for the unload event "inside" the iframe, and cancel the intent when this event occurs. But we could still have the case when the service fail to initialize before being able to listen to this event or cancelling the intent. So we need to be able to live with an "orphan" window listener on the client side.
  • The solution I implemented is that now every message brings the related intent id into a new type property. So now every message listener is no more listening to intent:ready but to something like {type: 'intent-77bcc42c-0fd8-11e7-ac95-8f605f6e8338:ready'}, to ensure that it will react to message targeting it only.
  • This have no effects to existing code, it is only internal implementation.

@gregorylegarec gregorylegarec force-pushed the feat/improve-intents branch from ceef063 to 9083ad6 Compare May 17, 2017 14:49
@@ -3,7 +3,7 @@ import {cozyFetchJSON} from './fetch'
const intentClass = 'coz-intent'

// inject iframe for service in given element
function injectService (url, element, data) {
function injectService (url, element, intent, data) {
const document = element.ownerDocument
if (!document) throw new Error('Cannot retrieve document object from given element')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why this validation code is not in the Promise constructor ?

I think you should not mix synchronous code and asynchronous code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it could be I think the reason is that we should do

const document = element.ownerDocument

twice, in two different part of the code. We may think about it in a next dedicated PR.

@ptbrowne
Copy link
Contributor

But we could still have the case when the service fail to initialize before being able to listen to this event or cancelling the intent. So we need to be able to live with an "orphan" window listener on the client side.

It is unclear for me to see why we cannot see that the service has failed.

@gregorylegarec
Copy link
Contributor Author

@ptbrowne For example : Client App A start an intent, App B handle this intent, so A loads the given B's service page in an iframe, but for whatever reason B fails miserably before being able to perform handshake. So in A, we still have a listener on B messages.

Copy link
Contributor

@y-lohse y-lohse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Good catch.
I'm guessing the event.origin is only the host and port, not the full URL, and that's why we can't use it to desambiguate the intent:ready events?

Copy link
Contributor

@y-lohse y-lohse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise cool. It's a shame for the added complexity, but at least it's not visible from the outside. :shipit:

@ptbrowne ptbrowne merged commit 5ab114d into cozy:master May 17, 2017
@ptbrowne
Copy link
Contributor

Understood. Thanks !

@gregorylegarec
Copy link
Contributor Author

@y-lohse Yep, you're right, event.origin only gives host and port, not the query string.

@gregorylegarec gregorylegarec deleted the feat/improve-intents branch May 30, 2017 22:50
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.

4 participants