-
Notifications
You must be signed in to change notification settings - Fork 61
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
Add connection #96
base: master
Are you sure you want to change the base?
Add connection #96
Conversation
Fixes Aaronius#58 Add connection lost and connection established events to Penpal. * **Parent Connection**: - Add `onConnectionLost` and `onConnection` options to `connectToChild` in `src/parent/connectToChild.ts`. - Call `onConnectionLost` when the connection with the child is lost. - Call `onConnection` when the connection with the child is established or re-established. * **Child Connection**: - Add `onConnectionLost` and `onConnection` options to `connectToParent` in `src/child/connectToParent.ts`. - Call `onConnectionLost` when the connection with the parent is lost. - Call `onConnection` when the connection with the parent is established or re-established. * **Documentation**: - Update `README.md` to include `onConnectionLost` and `onConnection` options for `connectToChild`. - Update `README.md` to include `onConnectionLost` and `onConnection` options for `connectToParent`. * **Tests**: - Add tests in `test/connectionManagement.spec.js` to verify `onConnectionLost` and `onConnection` functionality for `connectToChild`. - Add tests in `test/connectionManagement.spec.js` to verify `onConnectionLost` and `onConnection` functionality for `connectToParent`. --- For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/Aaronius/penpal/issues/58?shareId=XXXX-XXXX-XXXX-XXXX).
Hi @Kohart, thanks for the pull request! If you don't mind me asking, how do you intend to use this new functionality? In other words, what action would you be taking in your application when |
connection.promise.then(() => { | ||
document.body.removeChild(iframe); | ||
}); | ||
}); |
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.
This test is failing for me because it times out after 5 seconds (5 seconds is the Jasmine test timeout period). I believe the test isn't completing in time because removing the iframe doesn't immediately (or even quickly) destroy the connection. The code that destroys the connection when the iframe is removed can be found in monitorIframeRemoval.ts. Notice that monitorIframeRemoval.ts only checks to see if the iframe is removed every 60 seconds. If the iframe is not found at that point, then it destroys the connection.
A somewhat naive or partial approach to address this problem would be either of these options:
- Decrease the interval (currently 60 seconds) that checks whether the iframe is removed.
- Use a mock clock within the test and advance it by 60 seconds after removing the iframe (see the doesn't destroy connection if connection succeeds then timeout passes (connectToChild) test for an example of using a fake clock).
However, there's a bigger question that needs to be addressed as part of #58, which is: What constitutes the connection being lost? I'll expound on this in a comment at the PR level. The solution to this failing test depends on the answer to that question.
Before proceeding with this PR, we need to figure what constitutes a connection being lost. I'd like to hear your opinion, but let's continue that discussion in #58. It may be that wee need a heartbeat or something similar. |
Fixes #58
Add connection lost and connection established events to Penpal.
Parent Connection:
onConnectionLost
andonConnection
options toconnectToChild
insrc/parent/connectToChild.ts
.onConnectionLost
when the connection with the child is lost.onConnection
when the connection with the child is established or re-established.Child Connection:
onConnectionLost
andonConnection
options toconnectToParent
insrc/child/connectToParent.ts
.onConnectionLost
when the connection with the parent is lost.onConnection
when the connection with the parent is established or re-established.Documentation:
README.md
to includeonConnectionLost
andonConnection
options forconnectToChild
.README.md
to includeonConnectionLost
andonConnection
options forconnectToParent
.Tests:
test/connectionManagement.spec.js
to verifyonConnectionLost
andonConnection
functionality forconnectToChild
.test/connectionManagement.spec.js
to verifyonConnectionLost
andonConnection
functionality forconnectToParent
.For more details, open the Copilot Workspace session.