Uprade to comlink v4

[felixfbecker]

Closes #3989.

There were a bunch or renames, and StringMessageChannelAdapter is no longer bundled with comlink. There is a new experimental string channel support, that is less leaky.

Corresponding upstream comlink PR to fix types: GoogleChromeLabs/comlink#451

Tested manually in the webapp and browser extension.

[felixfbecker]

The test failure is a bug in Babel 😮

"use strict";

Object.defineProperty(exports, "__esModule", {
  value: true
});
exports.ExtLanguageFeatures = void 0;

var _comlink = require("@sourcegraph/comlink");

var _util = require("../../util");

var _common = require("./common");

var _types = require("./types");

/* eslint-disable no-sync */

/** @internal */
class ExtLanguageFeatures {
  constructor(proxy, documents) {
    this.proxy = _comlink.proxy; // should be proxy, not _comlink.proxy!
    this.documents = documents;
  }

[codecov]

Codecov Report

Merging #10149 into master will increase coverage by 0.00%.
The diff coverage is 67.46%.

@@           Coverage Diff           @@
##           master   #10149   +/-   ##
=======================================
  Coverage   42.77%   42.77%           
=======================================
  Files        1348     1348           
  Lines       74137    74136    -1     
  Branches     6651     6659    +8     
=======================================
+ Hits        31711    31712    +1     
+ Misses      39570    39568    -2     
  Partials     2856     2856           

Flag	Coverage Δ
#unit	42.77% <67.46%> (+<0.01%)	⬆️

Impacted Files	Coverage Δ
browser/src/extension/scripts/background.ts	0.00% <0.00%> (ø)
browser/src/platform/extensionHost.ts	28.57% <0.00%> (+7.73%)	⬆️
shared/src/api/extension/api/codeEditor.ts	83.33% <ø> (ø)
shared/src/api/extension/main.worker.ts	0.00% <0.00%> (ø)
shared/src/platform/context.ts	0.00% <0.00%> (ø)
shared/src/api/client/api/windows.ts	84.61% <33.33%> (ø)
shared/src/api/util.ts	76.92% <66.66%> (-1.34%)	⬇️
shared/src/api/client/api/languageFeatures.ts	82.35% <81.81%> (ø)
shared/src/api/extension/api/languageFeatures.ts	80.00% <83.33%> (ø)
shared/src/api/client/api/codeEditor.ts	83.33% <100.00%> (ø)
... and 22 more

[lguychard]

LGTM, I much prefer the local/remote naming 👍

[lguychard]

If we're going to use hasProperty a lot more, what would you think about having curriedHasProperty() (maybe there's a better name) and hasProperty()?

filter(curriedHasProperty('postMessage'))

// or
const isFoo = (bar: unknown) => hasProperty('foo', bar)

The hasProperty()() syntax is a bit alien/odd and I'd like to avoid odd patterns from proliferating in our codebase as much as possible.

[felixfbecker]

Yeah I thought about this too, and I had the same initial reaction. But I thought it would be weird/annoying to have the "curried" prefix and dual versions for every type guard too. One thought I had was that this may be one of these things that feel weird at first but you get used to it quickly.

Another thing I thought about was having a function to compose type guards, like all(...typeGuards)(value). That would avoid having to repeat the call with value for every type guard. I haven't found out how to type that yet though.

Ideally TypeScript would also just implement microsoft/TypeScript#21732 (which will happen eventually, given it has the label Committed), and then we can just use the in operator in non-functional contexts, while hasProperty() continues to serve the curried use case (and in that scenario, it would be nice if it doesn't need the "curried" prefix).

I propose we just leave it as-is for a bit and see where it goes (maybe improve with an all() helper, and/or maybe TS will ship in support sometime this year).