diff --git a/browser/src/extension/scripts/background.ts b/browser/src/extension/scripts/background.ts index 64c8d43caa9..3304a63dcc7 100644 --- a/browser/src/extension/scripts/background.ts +++ b/browser/src/extension/scripts/background.ts @@ -1,7 +1,7 @@ // We want to polyfill first. import '../polyfills' -import { Endpoint } from '@sourcegraph/comlink' +import { Endpoint } from 'comlink' import { without } from 'lodash' import { noop, Observable, Subscription } from 'rxjs' import { bufferCount, filter, groupBy, map, mergeMap, switchMap, take, concatMap } from 'rxjs/operators' @@ -19,6 +19,8 @@ import { observeSourcegraphURL } from '../../shared/util/context' import { assertEnv } from '../envAssertion' import { observeStorageKey, storage } from '../../browser/storage' import { isDefined } from '../../../../shared/src/util/types' +import { browserPortToMessagePort, findMessagePorts } from '../../platform/ports' +import { EndpointPair } from '../../../../shared/src/platform/context' const IS_EXTENSION = true @@ -188,7 +190,7 @@ async function main(): Promise { * This listens to events on browser.runtime.onConnect, pairs emitted ports using their naming pattern, * and emits pairs. Each pair of ports represents a connection with an instance of the content script. */ - const endpointPairs: Observable> = fromBrowserEvent( + const browserPortPairs: Observable> = fromBrowserEvent( browser.runtime.onConnect ).pipe( map(([port]) => port), @@ -226,40 +228,75 @@ async function main(): Promise { // when a port disconnects, the worker is terminated. This means there should always be exactly one // extension host worker per active instance of the content script. subscriptions.add( - endpointPairs.subscribe( - ({ proxy, expose }) => { - console.log('Extension host client connected') - // It's necessary to wrap endpoints because browser.runtime.Port objects do not support transferring MessagePorts. - // See https://github.com/GoogleChromeLabs/comlink/blob/master/messagechanneladapter.md - const { worker, clientEndpoints } = createExtensionHostWorker({ wrapEndpoints: true }) - const connectPortAndEndpoint = (port: browser.runtime.Port, endpoint: Endpoint): void => { - // False positive https://github.com/eslint/eslint/issues/12822 - // eslint-disable-next-line no-unused-expressions - endpoint.start?.() - port.onMessage.addListener(message => { - endpoint.postMessage(message) - }) - endpoint.addEventListener('message', event => { - port.postMessage((event as MessageEvent).data) - }) - } - // Connect proxy client endpoint - connectPortAndEndpoint(proxy, clientEndpoints.proxy) - // Connect expose client endpoint - connectPortAndEndpoint(expose, clientEndpoints.expose) - // Kill worker when either port disconnects - proxy.onDisconnect.addListener(() => worker.terminate()) - expose.onDisconnect.addListener(() => worker.terminate()) + browserPortPairs.subscribe({ + next: browserPortPair => { + subscriptions.add(handleBrowserPortPair(browserPortPair)) }, - err => { + error: err => { console.error('Error handling extension host client connection', err) - } - ) + }, + }) ) console.log('Sourcegraph background page initialized') } +/** + * Handle an incoming browser port pair coming from a content script. + */ +function handleBrowserPortPair( + browserPortPair: Record +): Subscription { + /** Subscriptions for this browser port pair */ + const subscriptions = new Subscription() + + console.log('Extension host client connected') + const { worker, clientEndpoints } = createExtensionHostWorker() + subscriptions.add(() => worker.terminate()) + + /** Forwards all messages between two endpoints (in one direction) */ + const forwardEndpoint = (from: Endpoint, to: Endpoint): void => { + const messageListener = (event: Event): void => { + const { data } = event as MessageEvent + to.postMessage(data, [...findMessagePorts(data)]) + } + from.addEventListener('message', messageListener) + subscriptions.add(() => from.removeEventListener('message', messageListener)) + + // False positive https://github.com/eslint/eslint/issues/12822 + // eslint-disable-next-line no-unused-expressions + from.start?.() + } + + const linkPortAndEndpoint = (role: keyof EndpointPair): void => { + const browserPort = browserPortPair[role] + const endpoint = clientEndpoints[role] + const tabId = browserPort.sender.tab?.id + if (!tabId) { + throw new Error('Expected Port to come from tab') + } + const link = browserPortToMessagePort(browserPort, `comlink-${role}-`, name => + browser.tabs.connect(tabId, { name }) + ) + subscriptions.add(link.subscription) + + forwardEndpoint(link.messagePort, endpoint) + forwardEndpoint(endpoint, link.messagePort) + + // Clean up when the port disconnects + const disconnectListener = subscriptions.unsubscribe.bind(subscriptions) + browserPort.onDisconnect.addListener(disconnectListener) + subscriptions.add(() => browserPort.onDisconnect.removeListener(disconnectListener)) + } + + // Connect proxy client endpoint + linkPortAndEndpoint('proxy') + // Connect expose client endpoint + linkPortAndEndpoint('expose') + + return subscriptions +} + // Browsers log this unhandled Promise automatically (and with a better stack trace through console.error) // eslint-disable-next-line @typescript-eslint/no-floating-promises main() diff --git a/browser/src/platform/extensionHost.ts b/browser/src/platform/extensionHost.ts index 722b1e9647d..da7ac85e32c 100644 --- a/browser/src/platform/extensionHost.ts +++ b/browser/src/platform/extensionHost.ts @@ -1,9 +1,9 @@ -import * as MessageChannelAdapter from '@sourcegraph/comlink/dist/umd/string-channel.experimental' -import { Observable } from 'rxjs' +import { Observable, Subscription } from 'rxjs' import * as uuid from 'uuid' import { EndpointPair } from '../../../shared/src/platform/context' import { isInPage } from '../context' import { SourcegraphIntegrationURLs } from './context' +import { browserPortToMessagePort } from './ports' function createInPageExtensionHost({ assetsURL, @@ -74,35 +74,20 @@ export function createExtensionHost(urls: Pick { - const proxyPort = browser.runtime.connect({ name: `proxy-${id}` }) - const exposePort = browser.runtime.connect({ name: `expose-${id}` }) - subscriber.next({ - proxy: endpointFromPort(proxyPort), - expose: endpointFromPort(exposePort), - }) - return () => { - proxyPort.disconnect() - exposePort.disconnect() - } - }) -} + // This is run in the content script + const subscription = new Subscription() + const setup = (role: keyof EndpointPair): MessagePort => { + const port = browser.runtime.connect({ name: `${role}-${id}` }) + subscription.add(() => port.disconnect()) -/** - * Partially wraps a browser.runtime.Port and returns a MessagePort created using - * comlink's {@link MessageChannelAdapter}, so that the Port can be used - * as a comlink Endpoint to transport messages between the content script and the extension host. - * - * It is necessary to wrap the port using MessageChannelAdapter because browser.runtime.Port objects do not support - * transferring MessagePort objects (see https://github.com/GoogleChromeLabs/comlink/blob/master/messagechanneladapter.md). - * - */ -function endpointFromPort(port: browser.runtime.Port): MessagePort { - return MessageChannelAdapter.wrap({ - send(data: string): void { - port.postMessage(data) - }, - addMessageListener(listener): void { - port.onMessage.addListener(listener) - }, + const link = browserPortToMessagePort(port, `comlink-${role}-`, name => browser.runtime.connect({ name })) + subscription.add(link.subscription) + return link.messagePort + } + subscriber.next({ + proxy: setup('proxy'), + expose: setup('expose'), + }) + return subscription }) } diff --git a/browser/src/platform/ports.ts b/browser/src/platform/ports.ts new file mode 100644 index 00000000000..2c7e3b35c6c --- /dev/null +++ b/browser/src/platform/ports.ts @@ -0,0 +1,252 @@ +import { isObject } from 'lodash' +import * as uuid from 'uuid' +import type { Message, MessageType } from 'comlink/dist/esm/protocol' +import { Subscription } from 'rxjs' + +/** Comlink enum value of release messages. */ +const RELEASE_MESSAGE_TYPE: MessageType.RELEASE = 5 + +/** + * Returns a `MessagePort` that was connected to the given `browser.runtime.Port` so that it can be used as an endpoint + * with comlink over browser extension script boundaries. + * + * `browser.runtime.Port` objects do not support transfering `MessagePort` objects, which comlink relies on. + * A new `browser.runtime.Port`, with an associated unique ID, will be created for each `MessagePort` transfer. + * The ID is added to the message and the original `MessagePort` is removed from it. + * + * @param browserPort The browser extension Port to link + * @param prefix A prefix unique to this call of `browserPortToMessagePort` (but the same on both sides) to prefix Port names. Incoming ports not matching this prefix will be ignored. + * @param connect A callback that is called to create a new connection to the other side of `browserPort` (e.g. through `browser.runtime.connect()` or `browser.tabs.connect()`). + */ +export function browserPortToMessagePort( + browserPort: browser.runtime.Port, + prefix: string, + connect: (name: string) => browser.runtime.Port +): { + messagePort: MessagePort + subscription: Subscription +} { + const rootSubscription = new Subscription() + + /** Browser ports waiting for a message referencing them, by their ID */ + const connectedBrowserPorts = new Map() + + /** Callbacks from messages that referenced a port ID, waiting for that port to connect */ + const waitingForPorts = new Map void>() + + // Listen to all incoming connections matching the prefix and memorize them + // to have them available when a message arrives that references them. + const connectListener = (incomingPort: browser.runtime.PortWithSender): void => { + if (!incomingPort.name.startsWith(prefix)) { + return + } + const id = incomingPort.name.slice(prefix.length) + const waitingFn = waitingForPorts.get(id) + if (waitingFn) { + waitingForPorts.delete(id) + waitingFn(incomingPort) + } else { + connectedBrowserPorts.set(id, incomingPort) + } + } + browser.runtime.onConnect.addListener(connectListener) + rootSubscription.add(() => browser.runtime.onConnect.removeListener(connectListener)) + + /** Run the given callback as soon as the given port ID is connected. */ + const whenConnected = (id: string, callback: (port: browser.runtime.Port) => void): void => { + const browserPort = connectedBrowserPorts.get(id) + if (browserPort) { + // We can delete the port from the Map after we set up a connection. + // There can never be the same ID twice and it is impossible to transfer the same MessagePort twice. + connectedBrowserPorts.delete(id) + callback(browserPort) + return + } + waitingForPorts.set(id, callback) + } + + /** + * Set up bi-directional listeners between the two ports that open new `browser.runtime.Port`s for transferred `MessagePort`s. + * + * @param browserPort The browser port used for communication with the other thread. + * @param adapterPort The `MessagePort` used to communicate with comlink. + */ + function link(browserPort: browser.runtime.Port, adapterPort: MessagePort): void { + const subscription = new Subscription(() => { + const browserPortId = browserPort.name.slice(prefix.length) + connectedBrowserPorts.delete(browserPortId) + waitingForPorts.delete(browserPortId) + // Close both ports. + adapterPort.close() + browserPort.disconnect() + }) + rootSubscription.add(subscription) + + const adapterListener = (event: MessageEvent): void => { + const data: Message = event.data + // Message from comlink needing to be forwarded to browser port with MessagePorts removed + const portRefs: PortRef[] = [] + // Find message port references and connect to the other side with the found IDs. + for (const { value, path, key, parent } of iteratePropertiesDeep(data)) { + if (value instanceof MessagePort) { + // Remove MessagePort from message + parent[key!] = null + // Open browser port in place of MessagePort + const id = uuid.v4() + const browserPort = connect(prefix + id) + link(browserPort, value) + // Include the ID of the browser port in the message + portRefs.push({ path, id }) + } + } + // Wrap message for the browser port to include all port IDs + const browserPortMessage: BrowserPortMessage = { message: data, portRefs } + + browserPort.postMessage(browserPortMessage) + + // Handle release messages (sent before the other end is closed) + // by cleaning up all Ports we control + if (data.type === RELEASE_MESSAGE_TYPE) { + subscription.unsubscribe() + } + } + adapterPort.addEventListener('message', adapterListener) + subscription.add(() => adapterPort.removeEventListener('message', adapterListener)) + + const browserPortListener = ({ message, portRefs }: BrowserPortMessage): void => { + const transfer: MessagePort[] = [] + for (const portRef of portRefs) { + const { port1: comlinkMessagePort, port2: intermediateMessagePort } = new MessageChannel() + + // Replace the port reference at the given path with a MessagePort that will be transferred. + replaceValueAtPath(message, portRef.path, comlinkMessagePort) + transfer.push(comlinkMessagePort) + + // Once the port with the mentioned ID is connected, link it up + whenConnected(portRef.id, browserPort => link(browserPort, intermediateMessagePort)) + } + + // Forward message, with MessagePorts + adapterPort.postMessage(message, transfer) + + // Handle release messages (sent before the other end is closed) + // by cleaning up all Ports we control + if (message.type === RELEASE_MESSAGE_TYPE) { + subscription.unsubscribe() + } + } + browserPort.onMessage.addListener(browserPortListener) + subscription.add(() => browserPort.onMessage.removeListener(browserPortListener)) + + const disconnectListener = subscription.unsubscribe.bind(subscription) + browserPort.onDisconnect.addListener(disconnectListener) + subscription.add(() => browserPort.onDisconnect.removeListener(disconnectListener)) + + adapterPort.start() + } + + const { port1: returnedMessagePort, port2: adapterPort } = new MessageChannel() + link(browserPort, adapterPort) + + return { + messagePort: returnedMessagePort, + subscription: rootSubscription, + } +} + +interface PortRef { + /** Path at which the MessagePort appeared. */ + path: Path + + /** UUID of the browser Port that was created for it. */ + id: string +} + +/** + * Message format used to communicate over `brower.runtime.Port`s. + */ +interface BrowserPortMessage { + /** + * The original comlink message, but with `MessagePort`s replaced with `null`. + */ + message: Message + + /** + * Where in the message `MessagePort`s were referenced and the ID of the `browser.runtime.Port` created for each. + */ + portRefs: PortRef[] +} + +type Key = string | number | symbol +type Path = Key[] + +interface PropertyIteratorEntry { + /** + * The current value. + */ + value: T + + /** + * The key of the current value in the parent object. Equivalent to `path[path.length - 1]`. + * `null` if the root value. + */ + key: Key | null + + /** + * The path of the current value in the root object. + */ + path: Path + + /** + * The parent object of the current value. `null` if the root object. + */ + parent: any | null +} + +/** + * Replace a value in an object structure at a given path with another value. + * + * @returns The old value at the path. + */ +function replaceValueAtPath(value: any, path: Path, newValue: unknown): unknown { + const lastProp = path[path.length - 1] + for (const prop of path.slice(0, -1)) { + value = value[prop] + } + const oldValue = value[lastProp] + value[lastProp] = newValue + return oldValue +} + +/** + * Iterate all properties of a given value or object recursively, starting with the value itself. + */ +export function* iteratePropertiesDeep( + value: T, + key: Key | null = null, + parent: any = null, + path: Path = [], + visited = new WeakSet() +): Iterable { + yield { value, path, parent, key } + if (!isObject(value) || visited.has(value)) { + return + } + visited.add(value) + + const keys = Object.keys(value) as (keyof typeof value)[] + for (const key of keys) { + yield* iteratePropertiesDeep(value[key], key, value, [...path, key], visited) + } +} + +/** + * Yields all `MessagePort`s found deeply in a given object. + */ +export function* findMessagePorts(message: unknown): Iterable { + for (const { value } of iteratePropertiesDeep(message)) { + if (value instanceof MessagePort) { + yield value + } + } +} diff --git a/package.json b/package.json index e5cb2516af7..5cadfbd473c 100644 --- a/package.json +++ b/package.json @@ -237,13 +237,13 @@ "@sentry/browser": "^5.15.4", "@slimsag/react-shortcuts": "^1.2.1", "@sourcegraph/codeintellify": "^7.0.0", - "@sourcegraph/comlink": "^4.2.0-fork.2", "@sourcegraph/extension-api-classes": "^1.0.3", "@sourcegraph/extension-api-types": "link:packages/@sourcegraph/extension-api-types", "@sourcegraph/react-loading-spinner": "0.0.7", "@sqs/jsonc-parser": "^1.0.3", "bootstrap": "^4.4.1", "classnames": "^2.2.6", + "comlink": "^4.3.0", "copy-to-clipboard": "^3.3.1", "core-js": "^3.6.4", "d3-axis": "^1.0.12", diff --git a/shared/src/api/client/api/codeEditor.ts b/shared/src/api/client/api/codeEditor.ts index 77f51e0392b..c3fb3214a43 100644 --- a/shared/src/api/client/api/codeEditor.ts +++ b/shared/src/api/client/api/codeEditor.ts @@ -1,4 +1,4 @@ -import { ProxyMarked, proxyMarker } from '@sourcegraph/comlink' +import { ProxyMarked, proxyMarker } from 'comlink' import { TextDocumentDecoration } from '@sourcegraph/extension-api-types' import { flatten, values } from 'lodash' import { BehaviorSubject, Observable, Subscription } from 'rxjs' diff --git a/shared/src/api/client/api/commands.ts b/shared/src/api/client/api/commands.ts index 88ac753ef19..d15541bfde0 100644 --- a/shared/src/api/client/api/commands.ts +++ b/shared/src/api/client/api/commands.ts @@ -1,10 +1,12 @@ -import { ProxyMarked, proxy, proxyMarker } from '@sourcegraph/comlink' +import { ProxyMarked, proxy, proxyMarker, Remote } from 'comlink' import { Unsubscribable } from 'sourcegraph' import { CommandRegistry } from '../services/command' +import { Subscription } from 'rxjs' +import { ProxySubscription } from './common' /** @internal */ export interface ClientCommandsAPI extends ProxyMarked { - $registerCommand(name: string, command: (...args: any) => any): Unsubscribable & ProxyMarked + $registerCommand(name: string, command: Remote<((...args: any) => any) & ProxyMarked>): Unsubscribable & ProxyMarked $executeCommand(command: string, args: any[]): Promise } @@ -14,8 +16,11 @@ export class ClientCommands implements ClientCommandsAPI, ProxyMarked { constructor(private registry: CommandRegistry) {} - public $registerCommand(command: string, run: (...args: any) => any): Unsubscribable & ProxyMarked { - return proxy(this.registry.registerCommand({ command, run })) + public $registerCommand(command: string, run: Remote<(...args: any) => any>): Unsubscribable & ProxyMarked { + const subscription = new Subscription() + subscription.add(this.registry.registerCommand({ command, run })) + subscription.add(new ProxySubscription(run)) + return proxy(subscription) } public $executeCommand(command: string, args: any[]): Promise { diff --git a/shared/src/api/client/api/common.ts b/shared/src/api/client/api/common.ts index 3c0eac13c01..fd4a26ad40f 100644 --- a/shared/src/api/client/api/common.ts +++ b/shared/src/api/client/api/common.ts @@ -1,27 +1,70 @@ -import { Remote, proxyMarker } from '@sourcegraph/comlink' +import { Remote, proxyMarker, releaseProxy, ProxyMethods, ProxyMarked, proxy, UnproxyOrClone } from 'comlink' import { noop } from 'lodash' -import { from, Observable, observable, Subscription } from 'rxjs' -import { mergeMap } from 'rxjs/operators' +import { from, Observable, observable as symbolObservable, Subscription, Unsubscribable } from 'rxjs' +import { mergeMap, finalize } from 'rxjs/operators' import { Subscribable } from 'sourcegraph' import { ProxySubscribable } from '../../extension/api/common' import { syncSubscription } from '../../util' import { asError } from '../../../util/errors' +import { FeatureProviderRegistry } from '../services/registry' + +// We subclass because rxjs checks instanceof Subscription. +// By exposing a Subscription as the interface to release the proxy, +// the released/not released state is inspectable and other Subcriptions +// can be smart about releasing references when this Subscription is closed. +// Subscriptions notify parent Subscriptions when they are unsubscribed. + +/** + * A `Subscription` representing the `MessagePort` used by a comlink proxy. + * Unsubscribing will send a RELEASE message over the MessagePort, then close it and remove all event listeners from it. + */ +export class ProxySubscription extends Subscription { + constructor(proxy: Pick) { + super(() => { + const p = proxy + ;(proxy as any) = null // null out closure reference to proxy + p[releaseProxy]() + }) + } +} + +/** + * An object that is backed by a comlink Proxy and exposes its Subscription so consumers can release it. + */ +export interface ProxySubscribed { + readonly proxySubscription: Subscription +} + +/** + * An ordinary Observable linked to an Observable in another thread through a comlink Proxy. + */ +export interface RemoteObservable extends Observable, ProxySubscribed {} /** * When a Subscribable is returned from the other thread (wrapped with `proxySubscribable()`), * this thread gets a `Promise` for a `Subscribable` _proxy_ where `subscribe()` returns a `Promise`. - * This function wraps that proxy in a real Rx Observable where `subscribe()` returns an `Unsubscribable` directly as expected. + * This function wraps that proxy in a real Rx Observable where `subscribe()` returns a `Subscription` directly as expected. + * + * The returned Observable is augmented with the `releaseProxy` method from comlink to release the underlying `MessagePort`. * * @param proxyPromise The proxy to the `ProxyObservable` in the other thread + * @param addToSubscription If provided, directly adds the `ProxySubscription` to this Subscription. */ -export const wrapRemoteObservable = (proxyPromise: Promise>>): Observable => - from(proxyPromise).pipe( +export const wrapRemoteObservable = ( + proxyPromise: Promise>>, + addToSubscription?: Subscription +): RemoteObservable => { + const proxySubscription = new Subscription() + if (addToSubscription) { + addToSubscription.add(proxySubscription) + } + const observable = from(proxyPromise).pipe( mergeMap( - proxySubscribable => - // eslint-disable-next-line @typescript-eslint/consistent-type-assertions - ({ + (proxySubscribable): Subscribable => { + proxySubscription.add(new ProxySubscription(proxySubscribable)) + return { // Needed for Rx type check - [observable](): Subscribable { + [symbolObservable](): Subscribable { return this }, subscribe(...args: any[]): Subscription { @@ -47,6 +90,68 @@ export const wrapRemoteObservable = (proxyPromise: Promise) + } + } ) ) + return Object.assign(observable, { proxySubscription }) +} + +/** + * Releases the underlying MessagePort of a remote Observable when it completes or is unsubscribed from. + * + * Important: This will prevent resubscribing to the Observable. Only use this operator in a scope where it is known + * that no resubscriptions can happen after completion, e.g. in a `switchMap()` callback. + * + * Must be used as the first parameter to `pipe()`, because the source must be a `RemoteObservable`. + */ +export const finallyReleaseProxy = () => (source: Observable & Partial) => { + const { proxySubscription } = source + if (!proxySubscription) { + console.warn('finallyReleaseProxy() used on Observable without proxy subscription') + return source + } + return source.pipe(finalize(() => proxySubscription.unsubscribe())) +} + +/** + * Helper function to register a remote provider returning an Observable, proxied by comlink, in a provider registry. + * + * @param registry The registry to register the provider on. + * @param registrationOptions The registration options to pass to `registerProvider()` + * @param remoteProviderFunction The provider function in a remote thread, proxied by `comlink`. + * + * @returns A Subscription that can be proxied through comlink which will unregister the provider. + */ +export function registerRemoteProvider< + TRegistrationOptions, + TLocalProviderParams extends UnproxyOrClone, + TProviderParams, + TProviderResult +>( + registry: FeatureProviderRegistry< + TRegistrationOptions, + (params: TLocalProviderParams) => Observable + >, + registrationOptions: TRegistrationOptions, + remoteProviderFunction: Remote<((params: TProviderParams) => ProxySubscribable) & ProxyMarked> +): Unsubscribable & ProxyMarked { + // This subscription will unregister the provider when unsubscribed. + const subscription = new Subscription() + + subscription.add( + registry.registerProvider(registrationOptions, params => + // Wrap the remote, proxied Observable in an ordinary Observable + // and add its underlying proxy subscription to our subscription + // to release the proxy when the provider gets unregistered. + wrapRemoteObservable(remoteProviderFunction(params), subscription) + ) + ) + + // Track the underlying proxy subscription of the provider in our subscription + // so that the proxy gets released when the provider gets unregistered. + subscription.add(new ProxySubscription(remoteProviderFunction)) + + // Prepare the subscription to be proxied to the remote side. + return proxy(subscription) +} diff --git a/shared/src/api/client/api/configuration.ts b/shared/src/api/client/api/configuration.ts index 6cfb94bce60..283582c004c 100644 --- a/shared/src/api/client/api/configuration.ts +++ b/shared/src/api/client/api/configuration.ts @@ -1,4 +1,4 @@ -import { Remote, ProxyMarked, proxyMarker } from '@sourcegraph/comlink' +import { Remote, ProxyMarked, proxyMarker } from 'comlink' import { from, Subscription } from 'rxjs' import { switchMap } from 'rxjs/operators' import { isSettingsValid } from '../../../settings/settings' diff --git a/shared/src/api/client/api/content.ts b/shared/src/api/client/api/content.ts index d6f287a72fa..81b8f6d66fc 100644 --- a/shared/src/api/client/api/content.ts +++ b/shared/src/api/client/api/content.ts @@ -1,8 +1,8 @@ -import { Remote, ProxyMarked, proxy, proxyMarker } from '@sourcegraph/comlink' +import { Remote, ProxyMarked, proxyMarker } from 'comlink' import { LinkPreview, Unsubscribable } from 'sourcegraph' import { ProxySubscribable } from '../../extension/api/common' import { LinkPreviewProviderRegistry } from '../services/linkPreview' -import { wrapRemoteObservable } from './common' +import { registerRemoteProvider } from './common' /** @internal */ export interface ClientContentAPI extends ProxyMarked { @@ -17,6 +17,6 @@ export function createClientContent(registry: LinkPreviewProviderRegistry): Clie return { [proxyMarker]: true, $registerLinkPreviewProvider: (urlMatchPattern, providerFunction) => - proxy(registry.registerProvider({ urlMatchPattern }, url => wrapRemoteObservable(providerFunction(url)))), + registerRemoteProvider(registry, { urlMatchPattern }, providerFunction), } } diff --git a/shared/src/api/client/api/context.ts b/shared/src/api/client/api/context.ts index 1dda5bd44a8..39d31213a36 100644 --- a/shared/src/api/client/api/context.ts +++ b/shared/src/api/client/api/context.ts @@ -1,4 +1,4 @@ -import { ProxyMarked, proxyMarker } from '@sourcegraph/comlink' +import { ProxyMarked, proxyMarker } from 'comlink' import { ContextValues, Unsubscribable } from 'sourcegraph' /** @internal */ diff --git a/shared/src/api/client/api/extensions.ts b/shared/src/api/client/api/extensions.ts index 1be935a1d35..3ce359384cc 100644 --- a/shared/src/api/client/api/extensions.ts +++ b/shared/src/api/client/api/extensions.ts @@ -1,4 +1,4 @@ -import { Remote } from '@sourcegraph/comlink' +import { Remote } from 'comlink' import { from, Subscription } from 'rxjs' import { bufferCount, startWith } from 'rxjs/operators' import { ExtExtensionsAPI } from '../../extension/api/extensions' diff --git a/shared/src/api/client/api/languageFeatures.ts b/shared/src/api/client/api/languageFeatures.ts index 6f8d80109b2..5426750e3f7 100644 --- a/shared/src/api/client/api/languageFeatures.ts +++ b/shared/src/api/client/api/languageFeatures.ts @@ -1,4 +1,4 @@ -import { Remote, ProxyMarked, proxy, proxyMarker } from '@sourcegraph/comlink' +import { Remote, ProxyMarked, proxyMarker } from 'comlink' import { Hover, Location } from '@sourcegraph/extension-api-types' import { CompletionList, DocumentSelector, Unsubscribable } from 'sourcegraph' import { ProxySubscribable } from '../../extension/api/common' @@ -7,7 +7,7 @@ import { ProvideCompletionItemSignature } from '../services/completion' import { ProvideTextDocumentHoverSignature } from '../services/hover' import { TextDocumentLocationProviderIDRegistry, TextDocumentLocationProviderRegistry } from '../services/location' import { FeatureProviderRegistry } from '../services/registry' -import { wrapRemoteObservable } from './common' +import { registerRemoteProvider } from './common' /** @internal */ export interface ClientLanguageFeaturesAPI extends ProxyMarked { @@ -68,33 +68,21 @@ export class ClientLanguageFeatures implements ClientLanguageFeaturesAPI, ProxyM ((params: TextDocumentPositionParams) => ProxySubscribable) & ProxyMarked > ): Unsubscribable & ProxyMarked { - return proxy( - this.hoverRegistry.registerProvider({ documentSelector }, params => - wrapRemoteObservable(providerFunction(params)) - ) - ) + return registerRemoteProvider(this.hoverRegistry, { documentSelector }, providerFunction) } public $registerDefinitionProvider( documentSelector: DocumentSelector, providerFunction: Remote<((params: TextDocumentPositionParams) => ProxySubscribable) & ProxyMarked> ): Unsubscribable & ProxyMarked { - return proxy( - this.definitionRegistry.registerProvider({ documentSelector }, params => - wrapRemoteObservable(providerFunction(params)) - ) - ) + return registerRemoteProvider(this.definitionRegistry, { documentSelector }, providerFunction) } public $registerReferenceProvider( documentSelector: DocumentSelector, providerFunction: Remote<((params: TextDocumentPositionParams) => ProxySubscribable) & ProxyMarked> ): Unsubscribable & ProxyMarked { - return proxy( - this.referencesRegistry.registerProvider({ documentSelector }, params => - wrapRemoteObservable(providerFunction(params)) - ) - ) + return registerRemoteProvider(this.referencesRegistry, { documentSelector }, providerFunction) } public $registerLocationProvider( @@ -102,11 +90,7 @@ export class ClientLanguageFeatures implements ClientLanguageFeaturesAPI, ProxyM documentSelector: DocumentSelector, providerFunction: Remote<((params: TextDocumentPositionParams) => ProxySubscribable) & ProxyMarked> ): Unsubscribable & ProxyMarked { - return proxy( - this.locationRegistry.registerProvider({ id, documentSelector }, params => - wrapRemoteObservable(providerFunction(params)) - ) - ) + return registerRemoteProvider(this.locationRegistry, { id, documentSelector }, providerFunction) } public $registerCompletionItemProvider( @@ -115,10 +99,6 @@ export class ClientLanguageFeatures implements ClientLanguageFeaturesAPI, ProxyM ((params: TextDocumentPositionParams) => ProxySubscribable) & ProxyMarked > ): Unsubscribable & ProxyMarked { - return proxy( - this.completionItemsRegistry.registerProvider({ documentSelector }, params => - wrapRemoteObservable(providerFunction(params)) - ) - ) + return registerRemoteProvider(this.completionItemsRegistry, { documentSelector }, providerFunction) } } diff --git a/shared/src/api/client/api/roots.ts b/shared/src/api/client/api/roots.ts index da51f98c147..2ad7642fbac 100644 --- a/shared/src/api/client/api/roots.ts +++ b/shared/src/api/client/api/roots.ts @@ -1,4 +1,4 @@ -import { Remote } from '@sourcegraph/comlink' +import { Remote } from 'comlink' import { Subscription } from 'rxjs' import { ExtRootsAPI } from '../../extension/api/roots' import { WorkspaceService } from '../services/workspaceService' diff --git a/shared/src/api/client/api/search.ts b/shared/src/api/client/api/search.ts index 3045ef6673b..c60805e1e8a 100644 --- a/shared/src/api/client/api/search.ts +++ b/shared/src/api/client/api/search.ts @@ -1,4 +1,4 @@ -import { Remote, ProxyMarked, proxy, proxyMarker } from '@sourcegraph/comlink' +import { Remote, ProxyMarked, proxy, proxyMarker } from 'comlink' import { from } from 'rxjs' import { QueryTransformer, Unsubscribable } from 'sourcegraph' import { TransformQuerySignature } from '../services/queryTransformer' diff --git a/shared/src/api/client/api/views.ts b/shared/src/api/client/api/views.ts index 491892b7b71..8050428debd 100644 --- a/shared/src/api/client/api/views.ts +++ b/shared/src/api/client/api/views.ts @@ -1,6 +1,6 @@ -import * as comlink from '@sourcegraph/comlink' +import * as comlink from 'comlink' import { isEqual, omit } from 'lodash' -import { combineLatest, from, ReplaySubject, Unsubscribable, ObservableInput } from 'rxjs' +import { combineLatest, from, ReplaySubject, Unsubscribable, ObservableInput, Subscription } from 'rxjs' import { distinctUntilChanged, map, switchMap } from 'rxjs/operators' import { PanelView, View } from 'sourcegraph' import { ContributableViewContainer } from '../../protocol' @@ -10,7 +10,7 @@ import { PanelViewWithComponent, PanelViewProviderRegistry } from '../services/p import { Location } from '@sourcegraph/extension-api-types' import { MaybeLoadingResult } from '@sourcegraph/codeintellify' import { ProxySubscribable } from '../../extension/api/common' -import { wrapRemoteObservable } from './common' +import { wrapRemoteObservable, ProxySubscription } from './common' import { ViewService, ViewContexts } from '../services/viewService' /** @internal */ @@ -114,11 +114,14 @@ export class ClientViews implements ClientViewsAPI { ) => ProxySubscribable & comlink.ProxyMarked > ): Unsubscribable & comlink.ProxyMarked { - return comlink.proxy( + const subscription = new Subscription() + subscription.add( this.viewService.register(id, ContributableViewContainer.Directory, context => - wrapRemoteObservable(provider(context)) + wrapRemoteObservable(provider(context), subscription) ) ) + subscription.add(new ProxySubscription(provider)) + return comlink.proxy(subscription) } public $registerGlobalPageViewProvider( @@ -129,10 +132,13 @@ export class ClientViews implements ClientViewsAPI { ) => ProxySubscribable & comlink.ProxyMarked > ): Unsubscribable & comlink.ProxyMarked { - return comlink.proxy( + const subscription = new Subscription() + subscription.add( this.viewService.register(id, ContributableViewContainer.GlobalPage, context => - wrapRemoteObservable(provider(context)) + wrapRemoteObservable(provider(context), subscription) ) ) + subscription.add(new ProxySubscription(provider)) + return comlink.proxy(subscription) } } diff --git a/shared/src/api/client/api/windows.ts b/shared/src/api/client/api/windows.ts index 611cbec565b..03b48dec0c6 100644 --- a/shared/src/api/client/api/windows.ts +++ b/shared/src/api/client/api/windows.ts @@ -1,4 +1,4 @@ -import { ProxyMarked, proxy, proxyMarker } from '@sourcegraph/comlink' +import { ProxyMarked, proxy, proxyMarker } from 'comlink' import { Subject } from 'rxjs' import * as sourcegraph from 'sourcegraph' import { diff --git a/shared/src/api/client/connection.ts b/shared/src/api/client/connection.ts index c8f17a7b735..6ef1a43c0e2 100644 --- a/shared/src/api/client/connection.ts +++ b/shared/src/api/client/connection.ts @@ -1,4 +1,4 @@ -import * as comlink from '@sourcegraph/comlink' +import * as comlink from 'comlink' import { from, merge, Subject, Subscription, of } from 'rxjs' import { concatMap } from 'rxjs/operators' import { ContextValues, Progress, ProgressOptions, Unsubscribable } from 'sourcegraph' diff --git a/shared/src/api/client/services/hover.ts b/shared/src/api/client/services/hover.ts index 57a9fd274fa..a474692ca4c 100644 --- a/shared/src/api/client/services/hover.ts +++ b/shared/src/api/client/services/hover.ts @@ -8,6 +8,7 @@ import { TextDocumentPositionParams } from '../../protocol' import { DocumentFeatureProviderRegistry } from './registry' import { isNot, isExactly } from '../../../util/types' import { MaybeLoadingResult, LOADING } from '@sourcegraph/codeintellify' +import { finallyReleaseProxy } from '../api/common' export type ProvideTextDocumentHoverSignature = ( params: TextDocumentPositionParams @@ -47,6 +48,7 @@ export function getHover( concat( [LOADING], provider(params).pipe( + finallyReleaseProxy(), defaultIfEmpty(null), catchError(err => { if (logErrors) { diff --git a/shared/src/api/client/services/linkPreview.ts b/shared/src/api/client/services/linkPreview.ts index 4de69f7fd47..4b00fd49db4 100644 --- a/shared/src/api/client/services/linkPreview.ts +++ b/shared/src/api/client/services/linkPreview.ts @@ -6,6 +6,7 @@ import { renderMarkdown } from '../../../util/markdown' import { combineLatestOrDefault } from '../../../util/rxjs/combineLatestOrDefault' import { property, isDefined } from '../../../util/types' import { FeatureProviderRegistry } from './registry' +import { finallyReleaseProxy } from '../api/common' interface MarkupContentPlainTextOnly extends Pick { kind: sourcegraph.MarkupKind.PlainText @@ -87,6 +88,7 @@ export function provideLinkPreview( providers.map(provider => from( provider(url).pipe( + finallyReleaseProxy(), catchError(err => { if (logErrors) { console.error(err) diff --git a/shared/src/api/client/services/location.ts b/shared/src/api/client/services/location.ts index 12ce1ceef92..c506109423e 100644 --- a/shared/src/api/client/services/location.ts +++ b/shared/src/api/client/services/location.ts @@ -1,5 +1,5 @@ import { Location } from '@sourcegraph/extension-api-types' -import { from, Observable, of, concat } from 'rxjs' +import { Observable, of, concat } from 'rxjs' import { catchError, map, switchMap, defaultIfEmpty } from 'rxjs/operators' import { combineLatestOrDefault } from '../../../util/rxjs/combineLatestOrDefault' import { TextDocumentPositionParams, TextDocumentRegistrationOptions } from '../../protocol' @@ -7,6 +7,7 @@ import { match, TextDocumentIdentifier } from '../types/textDocument' import { CodeEditorWithPartialModel } from './viewerService' import { DocumentFeatureProviderRegistry } from './registry' import { MaybeLoadingResult, LOADING } from '@sourcegraph/codeintellify' +import { finallyReleaseProxy } from '../api/common' /** * Function signature for retrieving related locations given a location (e.g., definition, implementation, and type @@ -134,14 +135,18 @@ export function getLocationsFromProviders< switchMap(providers => combineLatestOrDefault( providers.map(provider => - concat([LOADING], from(provider(params))).pipe( - defaultIfEmpty([]), - catchError(err => { - if (logErrors) { - console.error('Location provider errored:', err) - } - return [null] - }) + concat( + [LOADING], + provider(params).pipe( + finallyReleaseProxy(), + defaultIfEmpty([]), + catchError(err => { + if (logErrors) { + console.error('Location provider errored:', err) + } + return [null] + }) + ) ) ) ).pipe( diff --git a/shared/src/api/client/services/registry.ts b/shared/src/api/client/services/registry.ts index aefa9bf493a..99f2eea7c1f 100644 --- a/shared/src/api/client/services/registry.ts +++ b/shared/src/api/client/services/registry.ts @@ -1,4 +1,4 @@ -import { BehaviorSubject, Observable, Unsubscribable } from 'rxjs' +import { BehaviorSubject, Observable, Subscription } from 'rxjs' import { map } from 'rxjs/operators' import { getModeFromPath } from '../../../languages' import { parseRepoURI } from '../../../util/url' @@ -15,7 +15,7 @@ export interface Entry { export abstract class FeatureProviderRegistry { protected entries = new BehaviorSubject[]>([]) - public registerProvider(registrationOptions: O, provider: P): Unsubscribable { + public registerProvider(registrationOptions: O, provider: P): Subscription { return this.registerProviders([{ registrationOptions, provider }]) } @@ -24,13 +24,11 @@ export abstract class FeatureProviderRegistry { * at once means there is only one emission from {@link FeatureProviderRegistry#entries} (instead of one per * entry). */ - public registerProviders(entries: Entry[]): Unsubscribable { + public registerProviders(entries: Entry[]): Subscription { this.entries.next([...this.entries.value, ...entries]) - return { - unsubscribe: () => { - this.entries.next([...this.entries.value.filter(e => !entries.includes(e))]) - }, - } + return new Subscription(() => { + this.entries.next([...this.entries.value.filter(e => !entries.includes(e))]) + }) } /** All providers, emitted whenever the set of registered providers changed. */ diff --git a/shared/src/api/client/services/viewService.ts b/shared/src/api/client/services/viewService.ts index 3e8de30a574..b976467464b 100644 --- a/shared/src/api/client/services/viewService.ts +++ b/shared/src/api/client/services/viewService.ts @@ -1,9 +1,10 @@ -import { Observable, Unsubscribable, BehaviorSubject, of, combineLatest, concat } from 'rxjs' +import { Observable, BehaviorSubject, of, combineLatest, concat, Subscription } from 'rxjs' import { View as ExtensionView, DirectoryViewContext } from 'sourcegraph' import { switchMap, map, distinctUntilChanged, startWith, delay, catchError } from 'rxjs/operators' import { Evaluated, Contributions, ContributableViewContainer } from '../../protocol' import { isEqual } from 'lodash' import { asError, ErrorLike } from '../../../util/errors' +import { finallyReleaseProxy } from '../api/common' import { DeepReplace, isNot, isExactly } from '../../../util/types' /** @@ -33,7 +34,7 @@ export interface ViewService { id: string, where: W, provideView: ViewProviderFunction - ): Unsubscribable + ): Subscription /** * Get all providers for the given container. @@ -71,18 +72,16 @@ export const createViewService = (): ViewService => { const provider = { where, provideView } providers.value.set(id, provider as any) // TODO: find a type-safe way providers.next(providers.value) - return { - unsubscribe: () => { - const p = providers.value.get(id) - if (p?.provideView === provideView) { - // Check equality to ensure we only unsubscribe the exact same provider we - // registered, not some other provider that was registered later with the same - // ID. - providers.value.delete(id) - providers.next(providers.value) - } - }, - } + return new Subscription(() => { + const p = providers.value.get(id) + if (p?.provideView === provideView) { + // Check equality to ensure we only unsubscribe the exact same provider we + // registered, not some other provider that was registered later with the same + // ID. + providers.value.delete(id) + providers.next(providers.value) + } + }) }, getWhere: where => providers.pipe( @@ -93,7 +92,7 @@ export const createViewService = (): ViewService => { providers.pipe( map(providers => providers.get(id)?.provideView), distinctUntilChanged(), - switchMap(provider => (provider ? provider(context) : of(null))) + switchMap(provider => (provider ? provider(context).pipe(finallyReleaseProxy()) : of(null))) ), } } diff --git a/shared/src/api/extension/api/api.ts b/shared/src/api/extension/api/api.ts index 36a89a5435a..91f35110f92 100644 --- a/shared/src/api/extension/api/api.ts +++ b/shared/src/api/extension/api/api.ts @@ -1,4 +1,4 @@ -import { ProxyMarked } from '@sourcegraph/comlink' +import { ProxyMarked } from 'comlink' import { InitData } from '../extensionHost' import { ExtConfigurationAPI } from './configuration' import { ExtDocumentsAPI } from './documents' diff --git a/shared/src/api/extension/api/codeEditor.ts b/shared/src/api/extension/api/codeEditor.ts index 62e266aa99e..e65f339bcd3 100644 --- a/shared/src/api/extension/api/codeEditor.ts +++ b/shared/src/api/extension/api/codeEditor.ts @@ -1,4 +1,4 @@ -import { Remote } from '@sourcegraph/comlink' +import { Remote } from 'comlink' import { Range, Selection } from '@sourcegraph/extension-api-classes' import * as clientType from '@sourcegraph/extension-api-types' import { BehaviorSubject } from 'rxjs' diff --git a/shared/src/api/extension/api/commands.ts b/shared/src/api/extension/api/commands.ts index 28dff6eea24..be8c6002340 100644 --- a/shared/src/api/extension/api/commands.ts +++ b/shared/src/api/extension/api/commands.ts @@ -1,4 +1,4 @@ -import * as comlink from '@sourcegraph/comlink' +import * as comlink from 'comlink' import { Unsubscribable } from 'rxjs' import { ClientCommandsAPI } from '../../client/api/commands' import { syncSubscription } from '../../util' diff --git a/shared/src/api/extension/api/common.ts b/shared/src/api/extension/api/common.ts index 8881f015dce..526ad7c0b60 100644 --- a/shared/src/api/extension/api/common.ts +++ b/shared/src/api/extension/api/common.ts @@ -1,4 +1,4 @@ -import { Remote, ProxyMarked, proxy, proxyMarker, UnproxyOrClone } from '@sourcegraph/comlink' +import { Remote, ProxyMarked, proxy, proxyMarker, UnproxyOrClone } from 'comlink' import { from, isObservable, Observable, Observer, of } from 'rxjs' import { map } from 'rxjs/operators' import { ProviderResult, Subscribable, Unsubscribable } from 'sourcegraph' diff --git a/shared/src/api/extension/api/configuration.test.ts b/shared/src/api/extension/api/configuration.test.ts index 7f8acc850db..d227314d49d 100644 --- a/shared/src/api/extension/api/configuration.test.ts +++ b/shared/src/api/extension/api/configuration.test.ts @@ -1,6 +1,6 @@ import * as sinon from 'sinon' import { ExtConfiguration } from './configuration' -import { Remote, proxyMarker, createEndpoint, releaseProxy } from '@sourcegraph/comlink' +import { Remote, proxyMarker, createEndpoint, releaseProxy } from 'comlink' import { ClientConfigurationAPI } from '../../client/api/configuration' import { noop } from 'lodash' import { addProxyMethods } from '../../util' diff --git a/shared/src/api/extension/api/configuration.ts b/shared/src/api/extension/api/configuration.ts index 7fe5e421e42..47712c12772 100644 --- a/shared/src/api/extension/api/configuration.ts +++ b/shared/src/api/extension/api/configuration.ts @@ -1,4 +1,4 @@ -import { Remote, ProxyMarked, proxyMarker } from '@sourcegraph/comlink' +import { Remote, ProxyMarked, proxyMarker } from 'comlink' import { ReplaySubject } from 'rxjs' import * as sourcegraph from 'sourcegraph' import { SettingsCascade } from '../../../settings/settings' diff --git a/shared/src/api/extension/api/content.ts b/shared/src/api/extension/api/content.ts index 6c9e681b061..19a79954223 100644 --- a/shared/src/api/extension/api/content.ts +++ b/shared/src/api/extension/api/content.ts @@ -1,4 +1,4 @@ -import * as comlink from '@sourcegraph/comlink' +import * as comlink from 'comlink' import { Unsubscribable } from 'rxjs' import { LinkPreviewProvider } from 'sourcegraph' import { ClientContentAPI } from '../../client/api/content' diff --git a/shared/src/api/extension/api/context.ts b/shared/src/api/extension/api/context.ts index b1be5af988c..b8191e98fde 100644 --- a/shared/src/api/extension/api/context.ts +++ b/shared/src/api/extension/api/context.ts @@ -1,4 +1,4 @@ -import { Remote } from '@sourcegraph/comlink' +import { Remote } from 'comlink' import { ContextValues } from 'sourcegraph' import { ClientContextAPI } from '../../client/api/context' diff --git a/shared/src/api/extension/api/documents.ts b/shared/src/api/extension/api/documents.ts index 178ef1bb330..01598b6340a 100644 --- a/shared/src/api/extension/api/documents.ts +++ b/shared/src/api/extension/api/documents.ts @@ -1,4 +1,4 @@ -import { ProxyMarked, proxyMarker } from '@sourcegraph/comlink' +import { ProxyMarked, proxyMarker } from 'comlink' import { Subject } from 'rxjs' import { TextDocument } from 'sourcegraph' import { TextModelUpdate } from '../../client/services/modelService' diff --git a/shared/src/api/extension/api/extensions.ts b/shared/src/api/extension/api/extensions.ts index eaaec9716ba..c484a17b444 100644 --- a/shared/src/api/extension/api/extensions.ts +++ b/shared/src/api/extension/api/extensions.ts @@ -1,4 +1,4 @@ -import { ProxyMarked, proxyMarker } from '@sourcegraph/comlink' +import { ProxyMarked, proxyMarker } from 'comlink' import { Subscription, Unsubscribable } from 'rxjs' import { asError } from '../../../util/errors' import { tryCatchPromise } from '../../util' diff --git a/shared/src/api/extension/api/languageFeatures.ts b/shared/src/api/extension/api/languageFeatures.ts index ab573c234bf..0468f8598bd 100644 --- a/shared/src/api/extension/api/languageFeatures.ts +++ b/shared/src/api/extension/api/languageFeatures.ts @@ -1,5 +1,5 @@ /* eslint-disable no-sync */ -import * as comlink from '@sourcegraph/comlink' +import * as comlink from 'comlink' import * as clientType from '@sourcegraph/extension-api-types' import { Unsubscribable } from 'rxjs' import { diff --git a/shared/src/api/extension/api/roots.ts b/shared/src/api/extension/api/roots.ts index 940ff9d1b97..c24aa70b597 100644 --- a/shared/src/api/extension/api/roots.ts +++ b/shared/src/api/extension/api/roots.ts @@ -1,4 +1,4 @@ -import { ProxyMarked, proxyMarker } from '@sourcegraph/comlink' +import { ProxyMarked, proxyMarker } from 'comlink' import * as clientType from '@sourcegraph/extension-api-types' import { Subject } from 'rxjs' import * as sourcegraph from 'sourcegraph' diff --git a/shared/src/api/extension/api/search.ts b/shared/src/api/extension/api/search.ts index 0379a83036f..e1357dd9d85 100644 --- a/shared/src/api/extension/api/search.ts +++ b/shared/src/api/extension/api/search.ts @@ -1,4 +1,4 @@ -import * as comlink from '@sourcegraph/comlink' +import * as comlink from 'comlink' import { Unsubscribable } from 'rxjs' import { QueryTransformer } from 'sourcegraph' import { ClientSearchAPI } from '../../client/api/search' diff --git a/shared/src/api/extension/api/views.ts b/shared/src/api/extension/api/views.ts index 329f5782667..d03ed767325 100644 --- a/shared/src/api/extension/api/views.ts +++ b/shared/src/api/extension/api/views.ts @@ -1,4 +1,4 @@ -import * as comlink from '@sourcegraph/comlink' +import * as comlink from 'comlink' import * as sourcegraph from 'sourcegraph' import { ClientViewsAPI, PanelUpdater, PanelViewData } from '../../client/api/views' import { syncSubscription } from '../../util' diff --git a/shared/src/api/extension/api/windows.ts b/shared/src/api/extension/api/windows.ts index 04e9bd1f318..2ab2212ddf0 100644 --- a/shared/src/api/extension/api/windows.ts +++ b/shared/src/api/extension/api/windows.ts @@ -1,4 +1,4 @@ -import { Remote, ProxyMarked, proxyMarker } from '@sourcegraph/comlink' +import { Remote, ProxyMarked, proxyMarker } from 'comlink' import { sortBy } from 'lodash' import { BehaviorSubject, Observable, of } from 'rxjs' import * as sourcegraph from 'sourcegraph' diff --git a/shared/src/api/extension/extensionHost.ts b/shared/src/api/extension/extensionHost.ts index 82bb062fe0b..5203c2b32ab 100644 --- a/shared/src/api/extension/extensionHost.ts +++ b/shared/src/api/extension/extensionHost.ts @@ -1,4 +1,4 @@ -import * as comlink from '@sourcegraph/comlink' +import * as comlink from 'comlink' import { Location, MarkupKind, Position, Range, Selection } from '@sourcegraph/extension-api-classes' import { Subscription, Unsubscribable } from 'rxjs' import * as sourcegraph from 'sourcegraph' @@ -122,6 +122,8 @@ function createExtensionAPI( /** Proxy to main thread */ const proxy = comlink.wrap(endpoints.proxy) + ;(endpoints.proxy as any).role = 'proxy' + ;(endpoints.proxy as any).side = 'ext-host' // For debugging/tests. const sync = async (): Promise => { diff --git a/shared/src/api/extension/main.worker.ts b/shared/src/api/extension/main.worker.ts index 2464a938c80..b03de2eab87 100644 --- a/shared/src/api/extension/main.worker.ts +++ b/shared/src/api/extension/main.worker.ts @@ -1,9 +1,8 @@ import '../../polyfills' -import * as MessageChannelAdapter from '@sourcegraph/comlink/dist/umd/string-channel.experimental' import { fromEvent } from 'rxjs' import { take } from 'rxjs/operators' -import { EndpointPair, isEndpointPair } from '../../platform/context' +import { isEndpointPair } from '../../platform/context' import { startExtensionHost } from './extensionHost' import { hasProperty } from '../../util/types' @@ -12,33 +11,11 @@ interface InitMessage { proxy: MessagePort expose: MessagePort } - /** - * Whether the endpoints should be wrapped with a comlink {@link MessageChannelAdapter}. - * - * This is true when the messages passed on the endpoints are forwarded to/from - * other wrapped endpoints, like in the browser extension. - */ - wrapEndpoints: boolean } const isInitMessage = (value: unknown): value is InitMessage => typeof value === 'object' && value !== null && hasProperty('endpoints')(value) && isEndpointPair(value.endpoints) -const wrapMessagePort = (port: MessagePort): MessagePort => - MessageChannelAdapter.wrap({ - send: data => port.postMessage(data), - addMessageListener: listener => port.addEventListener('message', event => listener(event.data)), - }) - -const wrapEndpoints = ({ proxy, expose }: InitMessage['endpoints']): EndpointPair => { - proxy.start() - expose.start() - return { - proxy: wrapMessagePort(proxy), - expose: wrapMessagePort(expose), - } -} - /** * The entrypoint for the JavaScript context that runs the extension host (and all extensions). * @@ -51,7 +28,7 @@ async function extensionHostMain(): Promise { throw new Error('First message event in extension host worker was not a well-formed InitMessage') } const { endpoints } = event.data - const extensionHost = startExtensionHost(event.data.wrapEndpoints ? wrapEndpoints(endpoints) : endpoints) + const extensionHost = startExtensionHost(endpoints) self.addEventListener('unload', () => extensionHost.unsubscribe()) } catch (err) { console.error('Error starting the extension host:', err) diff --git a/shared/src/api/extension/worker.ts b/shared/src/api/extension/worker.ts index fe9ed739d51..488e885afb0 100644 --- a/shared/src/api/extension/worker.ts +++ b/shared/src/api/extension/worker.ts @@ -2,19 +2,10 @@ import { Observable } from 'rxjs' import ExtensionHostWorker from 'worker-loader?inline&name=extensionHostWorker.bundle.js!./main.worker.ts' import { EndpointPair } from '../../platform/context' -interface ExtensionHostInitOptions { - /** - * Whether the endpoints should be wrapped with a comlink {@link MessageChannelAdapter}. - * - * This is true when the messages passed on the endpoints are forwarded to/from - * other wrapped endpoints, like in the browser extension. - */ - wrapEndpoints: boolean -} - -export function createExtensionHostWorker({ - wrapEndpoints, -}: ExtensionHostInitOptions): { worker: ExtensionHostWorker; clientEndpoints: EndpointPair } { +/** + * Creates a web worker with the extension host and sets up a bidirectional MessageChannel-based communication channel. + */ +export function createExtensionHostWorker(): { worker: ExtensionHostWorker; clientEndpoints: EndpointPair } { const clientAPIChannel = new MessageChannel() const extensionHostAPIChannel = new MessageChannel() const worker = new ExtensionHostWorker() @@ -22,7 +13,7 @@ export function createExtensionHostWorker({ proxy: clientAPIChannel.port2, expose: extensionHostAPIChannel.port2, } - worker.postMessage({ endpoints: workerEndpoints, wrapEndpoints }, Object.values(workerEndpoints)) + worker.postMessage({ endpoints: workerEndpoints }, Object.values(workerEndpoints)) const clientEndpoints = { proxy: extensionHostAPIChannel.port1, expose: clientAPIChannel.port1, @@ -30,9 +21,9 @@ export function createExtensionHostWorker({ return { worker, clientEndpoints } } -export function createExtensionHost({ wrapEndpoints }: ExtensionHostInitOptions): Observable { +export function createExtensionHost(): Observable { return new Observable(subscriber => { - const { clientEndpoints, worker } = createExtensionHostWorker({ wrapEndpoints }) + const { clientEndpoints, worker } = createExtensionHostWorker() subscriber.next(clientEndpoints) return () => worker.terminate() }) diff --git a/shared/src/api/util.ts b/shared/src/api/util.ts index cbe45c195b3..22eed02f412 100644 --- a/shared/src/api/util.ts +++ b/shared/src/api/util.ts @@ -1,11 +1,12 @@ import { - RemoteObject, ProxyMarked, transferHandlers, ProxyMethods, createEndpoint, releaseProxy, -} from '@sourcegraph/comlink' + TransferHandler, + Remote, +} from 'comlink' import { Subscription } from 'rxjs' import { Subscribable, Unsubscribable } from 'sourcegraph' import { hasProperty } from '../util/types' @@ -29,13 +30,12 @@ export const isURL = (value: unknown): value is URL => * Idempotent. */ export function registerComlinkTransferHandlers(): void { - transferHandlers.set('URL', { + const urlTransferHandler: TransferHandler = { canHandle: isURL, - // TODO the comlink types could be better here to avoid the any - // eslint-disable-next-line @typescript-eslint/no-unsafe-return, @typescript-eslint/no-unsafe-member-access - serialize: (url: any) => url.href, - deserialize: (urlString: any) => new URL(urlString), - }) + serialize: url => [url.href, []], + deserialize: urlString => new URL(urlString), + } + transferHandlers.set('URL', urlTransferHandler) } /** @@ -43,16 +43,16 @@ export function registerComlinkTransferHandlers(): void { * * @param subscriptionPromise A Promise for a Subscription proxied from the other thread */ -export const syncSubscription = ( - subscriptionPromise: Promise> -): Subscription => +export const syncSubscription = (subscriptionPromise: Promise>): Subscription => // We cannot pass the proxy subscription directly to Rx because it is a Proxy that looks like a function - new Subscription(() => { - // eslint-disable-next-line @typescript-eslint/no-floating-promises - subscriptionPromise.then(proxySubscription => { - // eslint-disable-next-line @typescript-eslint/no-floating-promises - proxySubscription.unsubscribe() - }) + // eslint-disable-next-line @typescript-eslint/no-misused-promises + new Subscription(async function (this: any) { + const subscriptionProxy = await subscriptionPromise + await subscriptionProxy.unsubscribe() + subscriptionProxy[releaseProxy]() + + this._unsubscribe = null // Workaround: rxjs doesn't null out the reference to this callback + ;(subscriptionPromise as any) = null }) /** diff --git a/shared/src/platform/context.ts b/shared/src/platform/context.ts index 2b592cd2044..0f84d7b59ca 100644 --- a/shared/src/platform/context.ts +++ b/shared/src/platform/context.ts @@ -1,4 +1,4 @@ -import { Endpoint } from '@sourcegraph/comlink' +import { Endpoint } from 'comlink' import { NextObserver, Observable, Subscribable } from 'rxjs' import { SettingsEdit } from '../api/client/services/settings' import { GraphQLResult } from '../graphql/graphql' diff --git a/web/src/platform/context.ts b/web/src/platform/context.ts index cf27f8fa343..8d325928f9c 100644 --- a/web/src/platform/context.ts +++ b/web/src/platform/context.ts @@ -66,7 +66,7 @@ export function createPlatformContext(): PlatformContext { variables ), forceUpdateTooltip: () => Tooltip.forceUpdate(), - createExtensionHost: () => createExtensionHost({ wrapEndpoints: false }), + createExtensionHost: () => createExtensionHost(), urlToFile: toPrettyWebBlobURL, getScriptURLForExtension: bundleURL => bundleURL, sourcegraphURL: window.context.externalURL, diff --git a/yarn.lock b/yarn.lock index 0a692e812b0..bbdafc90ff3 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1971,11 +1971,6 @@ rxjs "^6.5.5" ts-key-enum "^2.0.0" -"@sourcegraph/comlink@^4.2.0-fork.2": - version "4.2.0-fork.2" - resolved "https://registry.npmjs.org/@sourcegraph/comlink/-/comlink-4.2.0-fork.2.tgz#5f6c02f4eec6d1971b80c27f6ecd9f04d7feb9bc" - integrity sha512-qhuu947hyGhni5F0aGwpToo548QT2irTDc6qqdRBIZPWCnO1oD4nG8dniP2EwNuX1CmoixRB4waRZgtN032Qcg== - "@sourcegraph/eslint-config@^0.16.0": version "0.16.0" resolved "https://registry.npmjs.org/@sourcegraph/eslint-config/-/eslint-config-0.16.0.tgz#360de690156c55a25e37f1c2ae99821f31d6de94" @@ -6379,6 +6374,11 @@ combined-stream@^1.0.6, combined-stream@~1.0.6: dependencies: delayed-stream "~1.0.0" +comlink@^4.3.0: + version "4.3.0" + resolved "https://registry.npmjs.org/comlink/-/comlink-4.3.0.tgz#80b3366baccd87897dab3638ebfcfae28b2f87c7" + integrity sha512-mu4KKKNuW8TvkfpW/H88HBPeILubBS6T94BdD1VWBXNXfiyqVtwUCVNO1GeNOBTsIswzsMjWlycYr+77F5b84g== + comma-separated-tokens@^1.0.0: version "1.0.5" resolved "https://registry.npmjs.org/comma-separated-tokens/-/comma-separated-tokens-1.0.5.tgz#b13793131d9ea2d2431cf5b507ddec258f0ce0db"