Skip to content
Snippets Groups Projects

Use browser extension Port objects instead of hacky comlink string channel

Merged Administrator requested to merge comlink-no-string-channel into master

Created by: felixfbecker

Comlink relies on creating MessagePorts for each proxied object and transferring those with postMessage(). But browser extension Port objects don't support transferring MessagePorts (or anything really).

Because of this, we used a hacky adapter that channeled every MessagePort through a single Port using string IDs (and serializing everything to a string, although that's not required).

This string message adapter was a hacky addition to comlink v3 that "should have never made it to master" according to its creator, and is missing in v4. There is an experimental one in v4, but it is broken, and actually broke our browser extension (I thought I had tested this, but I must have made a mistake, maybe I had a prod browser extension running? :shrug:‍♂️ Really wish we had tests for this in CI).

Because I couldn't get it fixed, I instead rethought our approach and worked on a solution that doesn't use a string wrapper. While Ports don't support transferables, it is possible to open multiple connections. How it works with this PR:

  • We capture all incoming Port connections and map them by their name.
  • Whenever we see a MessagePort in a message to be sent, we open a Port to the other side with a UUID name.
  • We include in the message sent over the Port the UUID and path at which the MessagePort appeared.
  • When we receive a message with UUID port references we take (or wait for) a connection with the UUID.
  • We hook that Port up to a MessagePort, and splice that MessagePort back into the message at the path it appeared.

@lguychard I hope I am not forgetting a reason why we didn't do this in the first place (besides saving effort) :sweat_smile:

Also would like to source the swarm intelligence to figure out where we need to add ensure resources are cleaned up and not leaked. Although I'm relatively sure we were leaking before - we're only using the new v4 [releaseProxy]() as of this PR.

Tested:

  • Chrome extension
  • Firefox extension
  • Web app
  • Native integration: Bitbucket server

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Created by: codecov[bot]

    Codecov Report

    Merging #10302 into master will not change coverage. The diff coverage is n/a.

    @@           Coverage Diff           @@
    ##           master   #10302   +/-   ##
    =======================================
      Coverage   44.38%   44.38%           
    =======================================
      Files        1410     1410           
      Lines       77432    77432           
      Branches     6938     6938           
    =======================================
      Hits        34372    34372           
      Misses      39839    39839           
      Partials     3221     3221           
    Flag Coverage Δ
    #go 47.33% <0.00%> (ø)
    #typescript 35.32% <0.00%> (ø)
    #unit 44.38% <0.00%> (ø)
  • Created by: felixfbecker

    :tada:

Please register or sign in to reply
Loading