Skip to content

The path option was added to the rescript.settings #478

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.

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

Merged
merged 9 commits into from
Jul 12, 2022
Merged
5 changes: 5 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,11 @@
"type": "boolean",
"default": false,
"description": "Automatically start ReScript's code analysis."
},
"rescript.settings.binaryPath": {
"type": ["string", "null"],
"default": null,
"description": "Path to the directory where ReScript binaries are. You can use it if you haven't or don't want to use the installed ReScript from node_modules in your project."
}
}
},
Expand Down
10 changes: 8 additions & 2 deletions server/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,19 @@ export let analysisProdPath = path.join(
"rescript-editor-analysis.exe"
);

export let rescriptBinName = "rescript";

export let bsbBinName = "bsb";

export let bscBinName = "bsc";

// can't use the native bsb/rescript since we might need the watcher -w flag, which is only in the JS wrapper
export let rescriptNodePartialPath = path.join(
"node_modules",
".bin",
"rescript"
rescriptBinName,
);
export let bsbNodePartialPath = path.join("node_modules", ".bin", "bsb");
export let bsbNodePartialPath = path.join("node_modules", ".bin", bsbBinName);

export let bsbLock = ".bsb.lock";
export let bsconfigPartialPath = "bsconfig.json";
Expand Down
44 changes: 40 additions & 4 deletions server/src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import * as v from "vscode-languageserver";
import * as rpc from "vscode-jsonrpc/node";
import * as path from "path";
import fs from "fs";
import os from "os";
// TODO: check DidChangeWatchedFilesNotification.
import {
DidOpenTextDocumentNotification,
Expand All @@ -24,9 +25,11 @@ import { filesDiagnostics } from "./utils";

interface extensionConfiguration {
askToStartBuild: boolean;
binaryPath: string | null;
}
let extensionConfiguration: extensionConfiguration = {
askToStartBuild: true,
binaryPath: null,
};
let pullConfigurationPeriodically: NodeJS.Timeout | null = null;

Expand Down Expand Up @@ -62,6 +65,11 @@ let codeActionsFromDiagnostics: codeActions.filesCodeActions = {};
// will be properly defined later depending on the mode (stdio/node-rpc)
let send: (msg: p.Message) => void = (_) => {};

let findBinary = (projectRootPath: p.DocumentUri) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Which binary?
findBuildBinary.

Copy link
Collaborator

Choose a reason for hiding this comment

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

All these functions, including findNodeBuildOfProjectRoot need to be renamed to contain the name Build.
So findBuildBinaryFromProjectRoot and findBuildBinaryFromConfig.

extensionConfiguration.binaryPath === null
? utils.findNodeBuildOfProjectRoot(projectRootPath)
: utils.findBinaryFromConfig(extensionConfiguration.binaryPath);

interface CreateInterfaceRequestParams {
uri: string;
}
Expand Down Expand Up @@ -232,7 +240,7 @@ let openedFile = (fileUri: string, fileContent: string) => {
// TODO: sometime stale .bsb.lock dangling. bsb -w knows .bsb.lock is
// stale. Use that logic
// TODO: close watcher when lang-server shuts down
if (utils.findNodeBuildOfProjectRoot(projectRootPath) != null) {
if (findBinary(projectRootPath) != null) {
let payload: clientSentBuildAction = {
title: c.startBuildAction,
projectRootPath: projectRootPath,
Expand All @@ -254,7 +262,20 @@ let openedFile = (fileUri: string, fileContent: string) => {
// handle in the isResponseMessage check in the message handling way
// below
} else {
// we should send something to say that we can't find bsb.exe. But right now we'll silently not do anything
let binaryPath =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't there already a function that does this: finding the binary directory?

extensionConfiguration.binaryPath === null
? path.join(projectRootPath, "node_modules", ".bin")
: extensionConfiguration.binaryPath;

let request: p.NotificationMessage = {
jsonrpc: c.jsonrpcVersion,
method: "window/showMessage",
params: {
type: p.MessageType.Error,
message: `Can't find ReScript binary on path ${binaryPath}`,
},
};
send(request);
}
}

Expand Down Expand Up @@ -593,7 +614,11 @@ function format(msg: p.RequestMessage): Array<p.Message> {
} else {
// code will always be defined here, even though technically it can be undefined
let code = getOpenedFileContent(params.textDocument.uri);
let formattedResult = utils.formatCode(filePath, code);
let formattedResult = utils.formatCode(
extensionConfiguration.binaryPath,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand this logic. The fact that there exists a configuration, which is optional, should not show up here.
The logic should be: find the binary path and use it. Either here or directly in the utils command.

filePath,
code
);
if (formattedResult.kind === "success") {
let max = code.length;
let result: p.TextEdit[] = [
Expand Down Expand Up @@ -933,6 +958,17 @@ function onMessage(msg: p.Message) {

if (initialConfiguration != null) {
extensionConfiguration = initialConfiguration;
if (
extensionConfiguration.binaryPath !== null &&
extensionConfiguration.binaryPath[0] === "~"
) {
// What should happen if the path contains the home directory symbol?
// This situation is handled below, but maybe it isn't the best option.
extensionConfiguration.binaryPath = path.join(
os.homedir(),
extensionConfiguration.binaryPath.slice(1)
);
}
}

send(response);
Expand Down Expand Up @@ -1041,7 +1077,7 @@ function onMessage(msg: p.Message) {
// TODO: close watcher when lang-server shuts down. However, by Node's
// default, these subprocesses are automatically killed when this
// language-server process exits
let found = utils.findNodeBuildOfProjectRoot(projectRootPath);
let found = findBinary(projectRootPath);
if (found != null) {
let bsbProcess = utils.runBuildWatcherUsingValidBuildPath(
found.buildPath,
Expand Down
75 changes: 55 additions & 20 deletions server/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,21 +70,47 @@ export let findBscNativeOfFile = (
}
};

let findBscBinFromConfig = (
pathToBinFromConfig: p.DocumentUri | null
): null | p.DocumentUri => {
if (pathToBinFromConfig === null) {
return null;
}
let bscPath = path.join(pathToBinFromConfig, c.bscBinName);
if (fs.existsSync(bscPath)) {
return bscPath;
}
return null;
};

// TODO: this doesn't handle file:/// scheme
export let findNodeBuildOfProjectRoot = (
projectRootPath: p.DocumentUri
): null | { buildPath: p.DocumentUri; isReScript: boolean } => {
let rescriptNodePath = path.join(projectRootPath, c.rescriptNodePartialPath);
let bsbNodePath = path.join(projectRootPath, c.bsbNodePartialPath);

if (fs.existsSync(rescriptNodePath)) {
return { buildPath: rescriptNodePath, isReScript: true };
} else if (fs.existsSync(bsbNodePath)) {
return { buildPath: bsbNodePath, isReScript: false };
let findBinaryBase = ({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a comment to explain what this does.

rescriptPath,
bsbPath,
}: {
rescriptPath: p.DocumentUri;
bsbPath: p.DocumentUri;
}): null | { buildPath: p.DocumentUri; isReScript: boolean } => {
if (fs.existsSync(rescriptPath)) {
return { buildPath: rescriptPath, isReScript: true };
} else if (fs.existsSync(bsbPath)) {
return { buildPath: bsbPath, isReScript: false };
}
return null;
};

export let findBinaryFromConfig = (pathToBinFromConfig: p.DocumentUri) =>
findBinaryBase({
rescriptPath: path.join(pathToBinFromConfig, c.rescriptBinName),
bsbPath: path.join(pathToBinFromConfig, c.bsbBinName),
});

export let findNodeBuildOfProjectRoot = (projectRootPath: p.DocumentUri) =>
findBinaryBase({
rescriptPath: path.join(projectRootPath, c.rescriptNodePartialPath),
bsbPath: path.join(projectRootPath, c.bsbNodePartialPath),
});

type execResult =
| {
kind: "success";
Expand All @@ -94,21 +120,30 @@ type execResult =
kind: "error";
error: string;
};
export let formatCode = (filePath: string, code: string): execResult => {

export let formatCode = (
pathToBinFromConfig: p.DocumentUri | null,
filePath: string,
code: string
): execResult => {
let extension = path.extname(filePath);
let formatTempFileFullPath = createFileInTempDir(extension);
fs.writeFileSync(formatTempFileFullPath, code, {
encoding: "utf-8",
});
try {
// Try to find the bsc bin from the binaryPath setting from the configuration.
let bscPath = findBscBinFromConfig(pathToBinFromConfig);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This logic is a bit strange. Seems biased towards the configuration. While the configuration is the exception not the rule.

The logic I would expect is:

  • find the normal binary path
  • if the config has a setting use that instead


// See comment on findBscNativeDirOfFile for why we need
// to recursively search for bsc.exe upward
let bscNativePath = findBscNativeOfFile(filePath);
bscPath = bscPath == null ? findBscNativeOfFile(filePath) : bscPath;

// Default to using the project formatter. If not, use the one we ship with
// the analysis binary in the extension itself.
if (bscNativePath != null) {
let result = childProcess.execFileSync(bscNativePath, [
// Default to using the formatter from the binaryPath setting from the configuration
// or the project formatter.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment is also inverted.
Default to use the binary path that ships with the analysis binary.
But if there is one in the config, override.

This is only about how the code reads, the semantics is the same.

// If not, use the one we ship with the analysis binary in the extension itself.
if (bscPath != null) {
let result = childProcess.execFileSync(bscPath, [
"-color",
"never",
"-format",
Expand Down Expand Up @@ -589,7 +624,7 @@ export let parseCompilerLogOutput = (
code: undefined,
severity: t.DiagnosticSeverity.Error,
tag: undefined,
content: [lines[i], lines[i+1]],
content: [lines[i], lines[i + 1]],
});
i++;
} else if (/^ +([0-9]+| +|\.) (│|┆)/.test(line)) {
Expand All @@ -603,9 +638,9 @@ export let parseCompilerLogOutput = (
// 10 ┆
} else if (line.startsWith(" ")) {
// part of the actual diagnostics message
parsedDiagnostics[parsedDiagnostics.length - 1].content.push(
line.slice(2)
);
parsedDiagnostics[parsedDiagnostics.length - 1].content.push(
line.slice(2)
);
} else if (line.trim() != "") {
// We'll assume that everything else is also part of the diagnostics too.
// Most of these should have been indented 2 spaces; sadly, some of them
Expand Down