Async rewrite #1093

mediremi · 2025-05-15T21:50:17Z

I made utils.findBinary async in order to allow us to get binary paths by dynamically importing the rescript package in the project root.

Every upstream function that needs to be async due to this change has been converted as well.

I've tested this on a v11 and a v12 project and everything seems to work fine.

….compileContents

…ectRoot(projectRootPath)

mediremi · 2025-05-15T23:36:54Z

server/src/utils.ts

+    // TODO: export `binPaths` from `rescript` package so that we don't need to
+    // copy the logic for figuring out `target`.
+    const target = `${process.platform}-${process.arch}`;
+    const targetPackagePath = path.join(rescriptDir, "..", `@rescript/${target}/bin.js`)
+    const { binPaths } = await import(targetPackagePath);


Unfortunately I couldn't just do import(rescriptDir) to get bin paths as the rescript package only exports ./lib/ and ./package.json in the exports field of its package.json.

We probably either want to export ./common/bins.js or have a default export that reexports binPaths.

As for @rescript/${target}, I had to append /bin.js to the import path for it to work as, like rescript, the per-platform binary packages have no default export.

Oh, you're right. I didn't re-export it because I wasn't sure the API from the compiler (it will be the first programmatic API, so...)

As for @rescript/${target}, I had to append /bin.js to the import path for it to work as, like rescript, the per-platform binary packages have no default export.

There is the exports field. Importing the binary package without the /bin.js suffix should also work.

Don't forget to use the right compilerOptions.nodeResolution option in the tsconfig.json. Use NodeNext as long as possible, or Bundler.

Don't forget to use the right compilerOptions.nodeResolution option in the tsconfig.json. Use NodeNext as long as possible, or Bundler.

Ah I didn't realise we were bundling as CJS, this explains the error messages I was getting when trying to import without bin.js. I'll have a go at tweaking compilerOptions.nodeResolution to make this work 👍

I tried the following:

compilerOptions.moduleResolution values NodeNext and Bundler

Updating typescript to v5.8.3

ES2022 instead of ES1029

but I still can't get it to work :(

This is still WIP, right? Or could we merge this and do the above in a follow up?

We can improve the dynamic import path in a followup PR 👍

mediremi · 2025-05-15T23:39:33Z

server/src/utils.ts

@@ -7,7 +7,9 @@ import {
  ResponseMessage,
 } from "vscode-languageserver-protocol";
 import fs from "fs";
+import fsAsync from "fs/promises";


In a later PR we might want to replace sync fs functions with their promise-based alternatives.

mediremi · 2025-05-15T23:44:25Z

package.json

@@ -255,9 +255,10 @@
  },
  "devDependencies": {
    "@types/node": "^14.14.41",
+    "@types/semver": "^7.7.0",


I added semver to make parsing the rescript version easier.

If adding this dependency is fine, then I can also refactor incrementalCompilation.figureOutBscArgs and utils.runAnalysisAfterSanityCheck to use it.

rescript-vscode/server/src/incrementalCompilation.ts

Line 576 in f4ba829

if (parseInt(project.rescriptVersion.split(".")[0] ?? "10") >= 11) {

rescript-vscode/server/src/utils.ts

Lines 211 to 220 in f4ba829

let shouldUseBuiltinAnalysis =

rescriptVersion?.startsWith("9.") ||

rescriptVersion?.startsWith("10.") ||

rescriptVersion?.startsWith("11.") ||

[

"12.0.0-alpha.1",

"12.0.0-alpha.2",

"12.0.0-alpha.3",

"12.0.0-alpha.4",

].includes(rescriptVersion ?? "");

I've done the proposed refactoring in this commit: d4cc19d

Great! 👍

cometkim · 2025-05-16T00:09:44Z

VSCode supports ESM-based extensions since very recently.
https://code.visualstudio.com/updates/v1_100#_esm-support-for-extensions

If we bump the target version and use fewer legacy dependencies, we can entirely switch to ESM.

Which accepts a fully asynchronous program, including top-level await, without bundler magic or TDZ-related bugs.

cometkim · 2025-05-16T00:15:27Z

server/src/utils.ts

+    // Can't use the native bsb/rescript since we might need the watcher -w
+    // flag, which is only in the JS wrapper
+    binaryPath = path.join(rescriptDir, rescriptJSWrapperPath)
+  } else if (semver.gte(rescriptVersion, "12.0.0-alpha.13")) {


AFAIK, npm's default semver comparison doesn't work intuitively with pre-releases. This might need the includePrelease flag.

https://www.npmjs.com/package/semver#prerelease-tags

You're right that we'll need that flag for range matching, so I added it in another place where we're checking major version numbers:

rescript-vscode/server/src/incrementalCompilation.ts

Line 580 in 22bff6d

semver.satisfies(project.rescriptVersion as string, ">=11", { includePrerelease: true })

For semver.gte(rescriptVersion, "12.0.0-alpha.13") it won't be needed as we're not doing a range comparison.

nojaf · 2025-05-16T06:13:23Z

Thank you for all this hard work!

zth · 2025-05-20T06:19:39Z

server/src/utils.ts

+// If ReScript < 12.0.0-alpha.13, then we want `{project_root}/node_modules/rescript/{c.platformDir}/{binary}`.
+// Otherwise, we want to dynamically import `{project_root}/node_modules/rescript` and from `binPaths` get the relevant binary.
+// We won't know which version is in the project root until we read and parse `{project_root}/node_modules/rescript/package.json`


👍 Good comment!

zth

Looks great, thank you very much! Let me know if this is good to merge as is or if there are ongoing things that need to be handled first.

mediremi · 2025-05-20T11:56:31Z

Thanks 😄 I'm happy for this to be merged as-is. We can try to figure out how to improve the dynamic import path in a followup PR 👍

zth · 2025-05-20T12:18:52Z

@mediremi awesome! One last thing - mind adding a changelog?

mediremi · 2025-05-20T12:24:03Z

Changelog entry added 👍

zth · 2025-05-20T12:37:09Z

Amazing work @mediremi!

mediremi added 25 commits May 15, 2025 22:49

Add semver dep

0b1fc72

Create async version of utils.findBinary

4af5bd7

Make findBscExeBinary and findEditorAnalysisBinary async

cecf7c4

Remove unused constants

df29b18

Make utils.runAnalysisAfterSanityCheck async

f4cde5a

Make parseCompilerLogOutput async

d729e3a

Make server.sendUpdatedDiagnostics async

f77e6dd

Make server.syncProjectConfigCache async

4612504

Make server.closedFile async

f642b24

Make server.rename async

e944890

Make server.completionResolve async

8ee0994

Trim trailing whitespace

015adaf

Use async allback to child_process.execFile in incrementalCompilation…

74178cb

….compileContents

Make server.updateDiagnosticSyntax async

f187e08

Make server.hover async

82b7cdb

Make server.inlayHint async

9430597

Make server.codeLens async

8150858

Make server.signatureHelp async

a3b5d14

Make server.definition async

9e9058e

Make server.typeDefinition async

f0de0d1

Make server.documentSymbol async

743dfcf

Make server.semanticTokens async

490dd35

Make server.completion async

2f85bc5

Make server.codeAction async

379a116

Make server.createInterface async

95ca8da

mediremi force-pushed the async-find-binary branch from 1ee276a to 95ca8da Compare May 15, 2025 22:39

mediremi added 4 commits May 15, 2025 23:40

Make utils.runAnalysisCommand async

cbe1239

Make utils.getReferencesForPosition async

28226f5

Make server.references async

90a573e

Make server.prepareRename async

032026e

mediremi added 6 commits May 15, 2025 23:48

Replace findReScriptVersion(filePath) with findReScriptVersionForProj…

a037a23

…ectRoot(projectRootPath)

Remove unused utils.findPlatformPath

190696d

Make server.findRescriptBinary async

2258d8e

Remove unused constant

628dc75

s/findBinaryAsync/findBinary

b3b26bf

Fix rescript binary package import path

40ecf3d

mediremi changed the title ~~[WIP] async find binary~~ Async rewrite May 15, 2025

mediremi commented May 15, 2025

View reviewed changes

mediremi marked this pull request as ready for review May 15, 2025 23:37

mediremi mentioned this pull request May 15, 2025

Find per-platform binaries for v12.0.0-alpha.13+ #1092

Merged

mediremi commented May 15, 2025

View reviewed changes

cometkim reviewed May 16, 2025

View reviewed changes

mediremi added 2 commits May 18, 2025 18:55

Move semver dependency from root package.json to server/package.json

da67038

Use semver package for version checks

d4cc19d

mediremi force-pushed the async-find-binary branch from f5bb3c6 to 28f1170 Compare May 18, 2025 19:56

Update typescript from v4.7.3 to v5.8.3

22bff6d

mediremi force-pushed the async-find-binary branch from 28f1170 to 22bff6d Compare May 18, 2025 19:56

zth reviewed May 20, 2025

View reviewed changes

zth approved these changes May 20, 2025

View reviewed changes

Add CHANGELOG

aa78b00

zth merged commit e045914 into rescript-lang:master May 20, 2025
6 checks passed

mediremi deleted the async-find-binary branch May 20, 2025 13:09

mediremi mentioned this pull request May 20, 2025

Restore semver package in top-level package.json to fix bump-version.js workflow #1097

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async rewrite #1093

Async rewrite #1093

mediremi commented May 15, 2025 •

edited

Loading

mediremi May 15, 2025

cometkim May 15, 2025

cometkim May 15, 2025 •

edited

Loading

mediremi May 16, 2025

mediremi May 18, 2025

zth May 20, 2025

mediremi May 20, 2025

mediremi May 15, 2025

mediremi May 15, 2025

mediremi May 18, 2025

zth May 20, 2025

cometkim commented May 16, 2025

cometkim May 16, 2025

mediremi May 18, 2025

nojaf commented May 16, 2025

zth May 20, 2025

zth left a comment

mediremi commented May 20, 2025

zth commented May 20, 2025

mediremi commented May 20, 2025

zth commented May 20, 2025

	let shouldUseBuiltinAnalysis =
	rescriptVersion?.startsWith("9.") \|\|
	rescriptVersion?.startsWith("10.") \|\|
	rescriptVersion?.startsWith("11.") \|\|
	[
	"12.0.0-alpha.1",
	"12.0.0-alpha.2",
	"12.0.0-alpha.3",
	"12.0.0-alpha.4",
	].includes(rescriptVersion ?? "");

Async rewrite #1093

Async rewrite #1093

Conversation

mediremi commented May 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cometkim May 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cometkim commented May 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojaf commented May 16, 2025

Choose a reason for hiding this comment

zth left a comment

Choose a reason for hiding this comment

mediremi commented May 20, 2025

zth commented May 20, 2025

mediremi commented May 20, 2025

zth commented May 20, 2025

mediremi commented May 15, 2025 •

edited

Loading

cometkim May 15, 2025 •

edited

Loading