Skip to content

Add workspace/didChangeConfiguration #528

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Add workspace/didChangeConfiguration #528

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ecc3393
workspace-did-change-config
aspeddro Jul 27, 2022
3feb330
update changelog
aspeddro Jul 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@

- Fix Incorrect semantic highlighting of `external` declarations https://github.com/rescript-lang/rescript-vscode/pull/517

#### :nail_care: Polish

- Don't restart server on change configuration `rescript.settings`. https://github.com/rescript-lang/rescript-vscode/pull/528
- Simplifies the `initializationOptions`. https://github.com/rescript-lang/rescript-vscode/pull/528

## v1.4.2

#### :bug: Bug Fix
Expand Down
4 changes: 2 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ You'll find all ReScript specific settings under the scope `rescript.settings`.
| Prompt to Start Build | If there's no ReScript build running already in the opened project, the extension will prompt you and ask if you want to start a build automatically. You can turn off this automatic prompt via the setting `rescript.settings.askToStartBuild`. |
| ReScript Binary Path | The extension will look for the existence of a `/node_modules/.bin/rescript` file and use its directory as the `binaryPath`. If it does not find it at the project root (which is where the nearest `bsconfig.json` resides), it goes up folders in the filesystem recursively until it either finds it (often the case in monorepos) or hits the top level. To override this lookup process, the path can be configured explicitly using the setting `rescript.settings.binaryPath` |
| Inlay Hints (experimental) | This allows an editor to place annotations inline with text to display type hints. Enable using `rescript.settings.inlayHints.enable: true` |
| Code Lens (experimental) | This tells the editor to add code lenses to function definitions, showing its full type above the definition. Enable using `rescript.settings.codeLens: true` |
| Code Lens (experimental) | This tells the editor to add code lenses to function definitions, showing its full type above the definition. Enable using `rescript.settings.codeLens.enable: true` |
Copy link
Contributor Author

@aspeddro aspeddro Jul 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setting in package.json is rescript.settings.codeLens.enable

| Autostarting the Code Analyzer | The Code Analyzer needs to be started manually by default. However, you can configure the extension to start the Code Analyzer automatically via the setting `rescript.settings.autoRunCodeAnalysis`. |

**Default settings:**
Expand All @@ -129,7 +129,7 @@ You'll find all ReScript specific settings under the scope `rescript.settings`.
"rescript.settings.inlayHints.maxLength": 25

// Enable (experimental) code lens for function definitions.
"rescript.settings.codeLens": true
"rescript.settings.codeLens.enable": true
```

## 🚀 Code Analyzer
Expand Down
19 changes: 5 additions & 14 deletions client/src/extension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,7 @@ export function activate(context: ExtensionContext) {
// We'll send the initial configuration in here, but this might be
// problematic because every consumer of the LS will need to mimic this.
// We'll leave it like this for now, but might be worth revisiting later on.
initializationOptions: {
extensionConfiguration: workspace.getConfiguration("rescript.settings"),
},
initializationOptions: workspace.getConfiguration("rescript.settings"),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I appreciate wanting to keep thing simple, it's a choice whether we want to break user's configuration now or later. What if we decide we want more things sent to the server from the client at a later stage? Could be as simple as meta data around what client it is, or whatever. If we make this change, we'd need to migrate back to the current solution again to support that, and that'd break all clients. I think the current approach is more future proof, even if it means sending an extra prop.

};

const client = new LanguageClient(
Expand Down Expand Up @@ -235,21 +233,14 @@ export function activate(context: ExtensionContext) {
// Start the client. This will also launch the server
client.start();

// Restart the language client automatically when certain configuration
// changes. These are typically settings that affect the capabilities of the
// language client, and because of that requires a full restart.
context.subscriptions.push(
workspace.onDidChangeConfiguration(({ affectsConfiguration }) => {
// Put any configuration that, when changed, requires a full restart of
// the server here. That will typically be any configuration that affects
// the capabilities declared by the server, since those cannot be updated
// on the fly, and require a full restart with new capabilities set when
// initializing.
if (
affectsConfiguration("rescript.settings.inlayHints") ||
affectsConfiguration("rescript.settings.codeLens")
affectsConfiguration("rescript.settings")
) {
commands.executeCommand("rescript-vscode.restart_language_server");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test this in an actual editor? Since this affects what capabilities the LS itself is initiated with, I can't imagine turning features on still works after this change, since you'd need to update the capabilities, and to do that you must restart the server.

client.sendNotification(
"workspace/didChangeConfiguration",
{ settings: workspace.getConfiguration("rescript.settings") }).catch((err) => { window.showErrorMessage(String(err)) })
Comment on lines +241 to +243
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I do like this touch of notifying the server on config changes, this in combination with removing the pulling breaks all other clients than VSCode, unless they also implement this pushing. I'd rather pull and just have it work on all clients without the extra work needed.

}
})
);
Expand Down
1 change: 0 additions & 1 deletion server/src/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,4 +52,3 @@ export let bsconfigModuleDefault = "commonjs";
export let bsconfigSuffixDefault = ".js";

export let configurationRequestId = "rescript_configuration_request";
export let pullConfigurationInterval = 10_000;
94 changes: 43 additions & 51 deletions server/src/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
InitializeParams,
InlayHintParams,
CodeLensParams,
DidChangeConfigurationParams,
} from "vscode-languageserver-protocol";
import * as utils from "./utils";
import * as codeActions from "./codeActions";
Expand All @@ -24,28 +25,31 @@ import { ChildProcess } from "child_process";
import { WorkspaceEdit } from "vscode-languageserver";
import { filesDiagnostics } from "./utils";

interface extensionConfiguration {
interface initializationOptions {
askToStartBuild: boolean;
inlayHints: {
enable: boolean;
maxLength: number | null;
};
codeLens: boolean;
codeLens: {
enable: boolean;
};
Comment on lines +34 to +36
Copy link
Contributor Author

@aspeddro aspeddro Jul 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setting in package.json is codeLens.enable

binaryPath: string | null;
}

// All values here are temporary, and will be overridden as the server is
// initialized, and the current config is received from the client.
let extensionConfiguration: extensionConfiguration = {
let initializationOptions: initializationOptions = {
askToStartBuild: true,
inlayHints: {
enable: false,
maxLength: 25,
},
codeLens: false,
codeLens: {
enable: false
},
binaryPath: null,
};
let pullConfigurationPeriodically: NodeJS.Timeout | null = null;

// https://microsoft.github.io/language-server-protocol/specification#initialize
// According to the spec, there could be requests before the 'initialize' request. Link in comment tells how to handle them.
Expand Down Expand Up @@ -99,9 +103,9 @@ let findBinaryDirPathFromProjectRoot = (
};

let getBinaryDirPath = (projectRootPath: p.DocumentUri) =>
extensionConfiguration.binaryPath == null
initializationOptions.binaryPath == null
? findBinaryDirPathFromProjectRoot(projectRootPath)
: extensionConfiguration.binaryPath;
: initializationOptions.binaryPath;

let findRescriptBinary = (projectRootPath: p.DocumentUri) =>
utils.findRescriptBinary(getBinaryDirPath(projectRootPath));
Expand Down Expand Up @@ -231,10 +235,10 @@ let compilerLogsWatcher = chokidar
.on("all", (_e, changedPath) => {
sendUpdatedDiagnostics();
sendCompilationFinishedMessage();
if (extensionConfiguration.inlayHints.enable === true) {
if (initializationOptions.inlayHints?.enable === true) {
sendInlayHintsRefresh();
}
if (extensionConfiguration.codeLens === true) {
if (initializationOptions.codeLens?.enable === true) {
sendCodeLensRefresh();
}
});
Expand Down Expand Up @@ -279,7 +283,7 @@ let openedFile = (fileUri: string, fileContent: string) => {
let bsbLockPath = path.join(projectRootPath, c.bsbLock);
if (
projectRootState.hasPromptedToStartBuild === false &&
extensionConfiguration.askToStartBuild === true &&
initializationOptions.askToStartBuild === true &&
!fs.existsSync(bsbLockPath)
) {
// TODO: sometime stale .bsb.lock dangling. bsb -w knows .bsb.lock is
Expand Down Expand Up @@ -399,6 +403,15 @@ function hover(msg: p.RequestMessage) {
}

function inlayHint(msg: p.RequestMessage) {

if (!initializationOptions.inlayHints?.enable) {
return {
jsonrpc: c.jsonrpcVersion,
id: msg.id,
result: null,
};
}
Comment on lines +407 to +413
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clear inlay hints when setting is disabled.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing this, and not restarting the server, means the client will continue to send messages to the server for these features. The point of turning the features off via a setting is also that they should be zero cost and not potentially slow down the extension. Even if this is just a null response, it'll still keep sending all of the messages back and forth. Might not hurt performance for inlay hints, but feature X, Y and Z that we'll add in the future might. Better to just turn it off completely via capabilities and a restart, like we're doing now. Then it's truly zero cost.


const params = msg.params as p.InlayHintParams;
const filePath = fileURLToPath(params.textDocument.uri);

Expand All @@ -409,7 +422,7 @@ function inlayHint(msg: p.RequestMessage) {
filePath,
params.range.start.line,
params.range.end.line,
extensionConfiguration.inlayHints.maxLength,
initializationOptions.inlayHints.maxLength,
],
msg
);
Expand All @@ -426,6 +439,15 @@ function sendInlayHintsRefresh() {
}

function codeLens(msg: p.RequestMessage) {

if (!initializationOptions.codeLens?.enable) {
return {
jsonrpc: c.jsonrpcVersion,
id: msg.id,
result: null,
};
}

Comment on lines +443 to +450
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clear codelens when setting is disabled

const params = msg.params as p.CodeLensParams;
const filePath = fileURLToPath(params.textDocument.uri);

Expand Down Expand Up @@ -569,24 +591,6 @@ function documentSymbol(msg: p.RequestMessage) {
return response;
}

function askForAllCurrentConfiguration() {
// https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_configuration
let params: p.ConfigurationParams = {
items: [
{
section: "rescript.settings",
},
],
};
let req: p.RequestMessage = {
jsonrpc: c.jsonrpcVersion,
id: c.configurationRequestId,
method: p.ConfigurationRequest.type.method,
params,
};
send(req);
}

function semanticTokens(msg: p.RequestMessage) {
// https://microsoft.github.io/language-server-protocol/specifications/specification-current/#textDocument_semanticTokens
let params = msg.params as p.SemanticTokensParams;
Expand Down Expand Up @@ -975,8 +979,8 @@ function onMessage(msg: p.Message) {
let params = msg.params as p.DidCloseTextDocumentParams;
closedFile(params.textDocument.uri);
} else if (msg.method === DidChangeConfigurationNotification.type.method) {
// Can't seem to get this notification to trigger, but if it does this will be here and ensure we're synced up at the server.
askForAllCurrentConfiguration();
let params = msg.params as DidChangeConfigurationParams
initializationOptions = params.settings
}
} else if (p.Message.isRequest(msg)) {
// request message, aka client sent request and waits for our mandatory reply
Expand All @@ -993,11 +997,10 @@ function onMessage(msg: p.Message) {
} else if (msg.method === "initialize") {
// Save initial configuration, if present
let initParams = msg.params as InitializeParams;
let initialConfiguration = initParams.initializationOptions
?.extensionConfiguration as extensionConfiguration | undefined;
let initialConfiguration = initParams.initializationOptions as initializationOptions | undefined;

if (initialConfiguration != null) {
extensionConfiguration = initialConfiguration;
initializationOptions = initialConfiguration;
}

// send the list of features we support
Expand Down Expand Up @@ -1035,12 +1038,10 @@ function onMessage(msg: p.Message) {
// TODO: Support range for full, and add delta support
full: true,
},
inlayHintProvider: extensionConfiguration.inlayHints.enable,
codeLensProvider: extensionConfiguration.codeLens
? {
workDoneProgress: false,
}
: undefined,
inlayHintProvider: true,
codeLensProvider: {
workDoneProgress: false
}
},
};
let response: p.ResponseMessage = {
Expand All @@ -1050,11 +1051,6 @@ function onMessage(msg: p.Message) {
};
initialized = true;

// Periodically pull configuration from the client.
pullConfigurationPeriodically = setInterval(() => {
askForAllCurrentConfiguration();
}, c.pullConfigurationInterval);

send(response);
} else if (msg.method === "initialized") {
// sent from client after initialize. Nothing to do for now
Expand Down Expand Up @@ -1082,10 +1078,6 @@ function onMessage(msg: p.Message) {
stopWatchingCompilerLog();
// TODO: delete bsb watchers

if (pullConfigurationPeriodically != null) {
clearInterval(pullConfigurationPeriodically);
}

let response: p.ResponseMessage = {
jsonrpc: c.jsonrpcVersion,
id: msg.id,
Expand Down Expand Up @@ -1154,10 +1146,10 @@ function onMessage(msg: p.Message) {
// without their settings overriding eachother. Not a problem now though
// as we'll likely only have "global" settings starting out.
let [configuration] = msg.result as [
extensionConfiguration | null | undefined
initializationOptions | null | undefined
];
if (configuration != null) {
extensionConfiguration = configuration;
initializationOptions = configuration;
}
}
} else if (
Expand Down