sourcegraph / sourcegraph-extension-api

Sourcegraph extension API: use and build extensions that enhance reading and reviewing code in your existing tools. "The extension API you wish your code host had."
44 stars 2 forks source link

improve configuration API #24

Closed sqs closed 6 years ago

codecov[bot] commented 6 years ago

Codecov Report

Merging #24 into master will increase coverage by 1.87%. The diff coverage is 70.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #24      +/-   ##
==========================================
+ Coverage   66.86%   68.73%   +1.87%     
==========================================
  Files          99      103       +4     
  Lines        3516     3653     +137     
  Branches      627      628       +1     
==========================================
+ Hits         2351     2511     +160     
+ Misses       1156     1140      -16     
+ Partials        9        2       -7
Impacted Files Coverage Δ
src/protocol/contribution.ts 100% <ø> (ø) :arrow_up:
src/client/client.ts 93.92% <ø> (ø) :arrow_up:
src/environment/registries.ts 100% <ø> (ø) :arrow_up:
src/protocol/configuration.ts 100% <ø> (ø) :arrow_up:
src/protocol/initialize.ts 100% <ø> (ø) :arrow_up:
src/extension/features2/textDocumentSync.ts 0% <0%> (ø)
src/client/features/configuration.ts 41.26% <0%> (ø) :arrow_up:
src/extension/api.ts 0% <0%> (ø)
src/extension/features2/common.ts 0% <0%> (ø)
src/extension/extension.ts 0% <0%> (ø)
... and 26 more

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 672ddc4...6cf3339. Read the comment docs.

chrismwendt commented 6 years ago

I often use the web worker example for testing purposes. Could that one be kept until there are links to example extensions in the readme?

Are optimistic configuration updates opt-in or always active? When might this be useful?

sqs commented 6 years ago

I often use the web worker example for testing purposes. Could that one be kept until there are links to example extensions in the readme?

Yeah, I will add them back when they are updated to the new extension API I am working on. You had a lot of good points about how to simplify how to write an extension (primarily the idea of 98% time not needing to call sendRequest or sendNotification, instead having a method like publishDecorations instead). This involves a lot of good changes.

Are optimistic configuration updates opt-in or always active? When might this be useful?

The principle I'm prototyping is, as above, making it so the primary extension API creates the feeling you are writing code to directly manipulate the client, i.e. hiding the fact that you're running in a different context (a Web Worker) that sends messages back and forth. This is like the VS Code API, which also presents an optimistic configuration API. You will still be able to drop down to the raw API by just getting the raw connection object (where you can manually register other CXP message listeners or raw send{Request,Notification}).

How does that sound?

sqs commented 6 years ago

@chrismwendt Here is the (draft, WIP) new TypeScript extension API, and extensions would get this CXP value in the arg to their main entrypoint func export function run(cxp: CXP): void { ... }. Then they would call cxp.configuration.get(...), cxp.decorations.add, etc. All this would of course be translated to CXP messages sent to the client.

import { Observable as BaseObservable } from 'rxjs'
import { MessageConnection } from '../jsonrpc2/connection'
import { Settings } from '../protocol'
import { URI } from '../types/textDocument'

/**
 * The CXP extension API, which extensions use to interact with the client.
 *
 * @template C the extension's settings
 */
export interface CXP<C = Settings> {
    /** The root URI of the workspace in which this extension is running. */
    root: URI | null

    /**
     * The configuration settings from the client.
     */
    configuration: Configuration

    /** The underlying CXP connection to the client. */
    readonly rawConnection: MessageConnection

    /**
     * Immediately stops the extension and closes the connection to the client.
     */
    close(): void
}

/**
 * A stream of values that can be transformed (with {@link Observable#pipe}) and subscribed to (with
 * {@link Observable#subscribe}).
 */
export interface Observable<T> {
    subscribe: BaseObservable<T>['subscribe']
    pipe: BaseObservable<T>['pipe']
}

/**
 * Configuration settings for a specific resource (such as a file, directory, or repository) or subject (such as a
 * user or organization, depending on the client).
 *
 * It may be merged from the following sources of settings, in order:
 *
 * - Default settings
 * - Global settings
 * - Organization settings (for all organizations the user is a member of)
 * - User settings
 * - Client settings
 * - Repository settings
 * - Directory settings
 *
 * @template C configuration type
 */
export interface Configuration<C> extends Observable<C> {
    /**
     * Returns a value from the configuration.
     *
     * @template K valid keys on the configuration object
     * @param key the name of the configuration property to get
     * @return the configuration value, or undefined
     */
    get<K extends keyof C>(key: K): C[K] | undefined

    /**
     * Observes changes to the configuration values for the given keys.
     *
     * @template K valid keys on the configuration object
     * @param keys the names of the configuration properties to observe
     * @return an observable that emits when any of the keys' values change (using deep comparison)
     */
    observe<K extends keyof C>(...keys: K[]): Observable<Pick<C, K>>

    /**
     * Updates the configuration value for the given key. The updated configuration value is sent to the client for
     * persistence.
     *
     * @template K valid keys on the configuration object
     * @param key the name of the configuration property to update
     * @param value the new value, or undefined to remove it
     * @return a promise that resolves when the client acknowledges the update
     */
    update<K extends keyof C>(key: K, value: C[K] | undefined): Promise<void>

    // TODO: Future plans:
    //
    // - add a way to read configuration from a specific scope (aka subject, but "scope" is probably a better word)
    // - describe how configuration defaults are supported
}
chrismwendt commented 6 years ago

Ok, and yeah, still being able to drop down to the underlying protocol is useful :+1:

How can we support running the same exact JS file both in a process node extension.js and in a web worker parcel serve extension.js with the export function activate()... API?

Introducing a wrapper cxp-cli would totally work, but I wonder if it's possible to change the API to this instead:

import { run } from 'cxp'

run((cxp: CXP) => { ... })

Internally, function run(cb) { ... } could check if require('fs') returns undefined (that's what parcel sets it to) to determine whether or not it's running in a web worker.

sqs commented 6 years ago

closing in favor of #25