Adoption of `PortProvider` and `PortFinder` #3903

esanzgar · 2021-11-03T18:06:52Z

This adoption simplifies the discovery of frames.

I did a little bit of clean-up in the tests: rearranged a few variables
alphabetically, and delete unneeded lines and variables.

Closes #3533

codecov · 2021-11-16T11:11:00Z

Codecov Report

Merging #3903 (75d7e43) into master (922d6ef) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3903      +/-   ##
==========================================
- Coverage   99.00%   99.00%   -0.01%     
==========================================
  Files         214      214              
  Lines        8015     8011       -4     
  Branches     1896     1894       -2     
==========================================
- Hits         7935     7931       -4     
  Misses         80       80

Impacted Files	Coverage Δ
src/annotator/annotation-sync.js	`100.00% <100.00%> (ø)`
src/annotator/guest.js	`99.14% <100.00%> (ø)`
src/annotator/sidebar.js	`98.18% <100.00%> (-0.04%)`	⬇️
src/shared/port-finder.js	`100.00% <100.00%> (ø)`
src/shared/port-provider.js	`100.00% <100.00%> (ø)`
src/sidebar/services/frame-sync.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 922d6ef...75d7e43. Read the comment docs.

robertknight

I have had an initial look over this. Functionally everything seems to work as desired. Aside from the minor comments, a few high level issues I raised:

Where we put the logic that determines where the host frame is, in relation to the current frame. Related to this, the behavior of the Guest constructor changes significantly depending on the config.subFrameIdentifier property, but that's not captured at all in docs or argument types.
Where the PortProvider is constructed. This currently happens in the Sidebar class, but perhaps it should live at a higher level (ie. the entry point) since it is so central to how the application works and should be easy to find.
I raised a hazard regarding stubbing of window.parent.postMessage in guests. That doesn't break anything in the test runner that I can see at present, but I can see a risk of that happening in future.

src/annotator/guest.js

src/annotator/sidebar.js

src/annotator/guest.js

src/annotator/index.js

src/annotator/test/integration/anchoring-test.js

esanzgar

@robertknight, I have added all your suggestions on a second commit.

src/annotator/guest.js

src/annotator/test/integration/anchoring-test.js

src/annotator/test/sidebar-test.js

esanzgar · 2021-11-18T08:46:53Z

src/shared/port-provider.js

@@ -178,7 +178,7 @@ export class PortProvider {
  /**
   * Returns the `host` port from the `sidebar-host` channel.
   */
-  get sidebarPort() {


I realised it had the wrong port name.

You'll need to change this if other other host-side ports can be retrieved in future, such as a host <-> notebook port. I think something like hostPortFor('sidebar') might be a good API even if the 'sidebar' argument is currently redundant, as it makes it clearer to a reader what is at the other end of the port.

esanzgar · 2021-11-18T08:55:06Z

src/shared/port-provider.js

    }
  }

  /**
-   * @param {'hostPortRequest'} eventName
+   * @param {'hostFrameRequest'} eventName


I believe this name matches the PortFinder#discover, which tries to discover a frame. The name of the port is not as relevant as that the host frame has been discovered.

robertknight

I had some suggestions regarding PortProvider APIs and a few JSDoc comments. Otherwise the changes look good to me.

src/annotator/index.js

src/annotator/sidebar.js

robertknight · 2021-11-19T09:33:35Z

src/shared/port-provider.js

@@ -178,7 +178,7 @@ export class PortProvider {
  /**
   * Returns the `host` port from the `sidebar-host` channel.
   */
-  get sidebarPort() {


You'll need to change this if other other host-side ports can be retrieved in future, such as a host <-> notebook port. I think something like hostPortFor('sidebar') might be a good API even if the 'sidebar' argument is currently redundant, as it makes it clearer to a reader what is at the other end of the port.

src/shared/port-provider.js

robertknight · 2021-11-19T09:47:54Z

src/sidebar/services/frame-sync.js

@@ -226,7 +234,7 @@ export class FrameSyncService {
  /**
   * Connect to the host frame and guest frame(s) in the current browser tab.
   */
-  connect() {
+  async connect() {


Because connect is now async, any failures will no longer trigger the catch block at the bottom of src/sidebar/index.js and thus the exception won't be reported via reportLaunchError. I think fixing that would be too big of a change for this PR, but we should address it in future.

This adoption simplifies the discovery of frames. I did a little bit of clean-up in the tests: rearranged a few variables alphabetically, and delete unneeded lines and variables.

In future PR, `Guest` will contain two bridges (like `FrameSync`). Naming them by the destination frame will help identify the purpose of it. It seems that the convention for this `Bridge`s is to contain the word `RPC` to indicate that it is a remote (inter-frame) procedure.

esanzgar changed the base branch from master to port-finder-and-port-provider November 3, 2021 18:07

esanzgar mentioned this pull request Nov 8, 2021

Implement PortProvider and PortFinder #3881

Merged

esanzgar force-pushed the port-finder-and-port-provider branch 3 times, most recently from cffd395 to c6e48bf Compare November 11, 2021 12:52

Base automatically changed from port-finder-and-port-provider to master November 15, 2021 14:57

esanzgar force-pushed the port-finder-and-port-provider-adoption branch from f0d7c4d to cacceeb Compare November 16, 2021 11:07

esanzgar force-pushed the port-finder-and-port-provider-adoption branch from cacceeb to 92b6ca3 Compare November 16, 2021 11:30

esanzgar changed the base branch from master to simplify-nomenclature November 16, 2021 11:31

esanzgar marked this pull request as ready for review November 16, 2021 11:32

esanzgar changed the title ~~POC: Port finder and port provider adoption~~ Adoption of PortProvider and PortFinder Nov 16, 2021

Base automatically changed from simplify-nomenclature to master November 17, 2021 10:21

robertknight requested changes Nov 17, 2021

View reviewed changes

esanzgar force-pushed the port-finder-and-port-provider-adoption branch 4 times, most recently from 8ce62d7 to 2a730d1 Compare November 18, 2021 09:02

esanzgar commented Nov 18, 2021

View reviewed changes

esanzgar requested a review from robertknight November 18, 2021 09:05

robertknight approved these changes Nov 19, 2021

View reviewed changes

Adopt new PortProvider and PortFinder

95ad644

This adoption simplifies the discovery of frames. I did a little bit of clean-up in the tests: rearranged a few variables alphabetically, and delete unneeded lines and variables.

esanzgar force-pushed the port-finder-and-port-provider-adoption branch 2 times, most recently from 7e200c7 to a461010 Compare November 22, 2021 11:17

esanzgar added 2 commits November 22, 2021 12:22

Apply suggestions from code review

da795e0

esanzgar force-pushed the port-finder-and-port-provider-adoption branch from a461010 to 75d7e43 Compare November 22, 2021 11:22

esanzgar merged commit 520b5c5 into master Nov 22, 2021

esanzgar deleted the port-finder-and-port-provider-adoption branch November 22, 2021 11:27

esanzgar mentioned this pull request Nov 22, 2021

Revert "Adoption of PortProvider and PortFinder" #3951

Merged

esanzgar mentioned this pull request Nov 23, 2021

Cleanup tasks after the new inter-frame communication adoption #3957

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adoption of `PortProvider` and `PortFinder` #3903

Adoption of `PortProvider` and `PortFinder` #3903

esanzgar commented Nov 3, 2021 •

edited

Loading

codecov bot commented Nov 16, 2021 •

edited

Loading

robertknight left a comment

esanzgar left a comment

esanzgar Nov 18, 2021

robertknight Nov 19, 2021

esanzgar Nov 18, 2021

robertknight left a comment

robertknight Nov 19, 2021

robertknight Nov 19, 2021

Adoption of PortProvider and PortFinder #3903

Adoption of PortProvider and PortFinder #3903

Conversation

esanzgar commented Nov 3, 2021 • edited Loading

codecov bot commented Nov 16, 2021 • edited Loading

Codecov Report

robertknight left a comment

Choose a reason for hiding this comment

esanzgar left a comment

Choose a reason for hiding this comment

esanzgar Nov 18, 2021

Choose a reason for hiding this comment

robertknight Nov 19, 2021

Choose a reason for hiding this comment

esanzgar Nov 18, 2021

Choose a reason for hiding this comment

robertknight left a comment

Choose a reason for hiding this comment

robertknight Nov 19, 2021

Choose a reason for hiding this comment

robertknight Nov 19, 2021

Choose a reason for hiding this comment

Adoption of `PortProvider` and `PortFinder` #3903

Adoption of `PortProvider` and `PortFinder` #3903

esanzgar commented Nov 3, 2021 •

edited

Loading

codecov bot commented Nov 16, 2021 •

edited

Loading