Add connection #96
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.
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:
onConnectionLostandonConnectionoptions toconnectToChildinsrc/parent/connectToChild.ts.onConnectionLostwhen the connection with the child is lost.onConnectionwhen the connection with the child is established or re-established.Child Connection:
onConnectionLostandonConnectionoptions toconnectToParentinsrc/child/connectToParent.ts.onConnectionLostwhen the connection with the parent is lost.onConnectionwhen the connection with the parent is established or re-established.Documentation:
README.mdto includeonConnectionLostandonConnectionoptions forconnectToChild.README.mdto includeonConnectionLostandonConnectionoptions forconnectToParent.Tests:
test/connectionManagement.spec.jsto verifyonConnectionLostandonConnectionfunctionality forconnectToChild.test/connectionManagement.spec.jsto verifyonConnectionLostandonConnectionfunctionality forconnectToParent.For more details, open the Copilot Workspace session.