Skip to content

Conversation

mediremi
Copy link
Member

rescript-lang/rescript#7395 split binaries into optional platform-specific dependencies (e.g. @rescript/linux-x64).

I've updated utils.findPlatformPath (used by utils.findBscExeBinary and utils.findEditorAnalysisBinary) to look for these platform-specific dependencies if node_modules/rescript/{platform} doesn't exist.

@zth
Copy link
Member

zth commented May 11, 2025

Thank you! 😄

@cometkim can you have a quick look at this one?

@cometkim
Copy link
Member

Can't we use rescript as an API?

If we already know the directory, we can use it as an API like:

const { binPaths } = await import('../path/to/rescript package')
binPaths.bsc_exe // should point to the binary path

@mediremi
Copy link
Member Author

The reason why I didn't go with the dynamic import approach is that findPlatformPath and its upstreams (e.g. utils.findBscExeBinary->server.openedFile->server.onMessage) are all synchronous functions and that making them async would likely be an extensive refactor.

I think that copying the import path logic of @rescript/${process.platform}-${process.arch} from cli/common/bins.js is a good pragmatic short-term solution, and a refactor to make the relevant functions async can be done later.

@nojaf
Copy link
Member

nojaf commented May 14, 2025

The entire LSP server is riddled with short-term fixes. I would recommend taking the plunge and going for the asynchronous rewrite. I've encountered this limitation before, and I genuinely believe it's the best approach. @zth, what are your thoughts?

@zth
Copy link
Member

zth commented May 14, 2025

@mediremi would it be possible for you to do a quick assessment on how much work it'd be to do the async refactor? At some point we'll need to bite the bullet for sure. But whether it's best to do it now or later is not clear to me right now.

@mediremi
Copy link
Member Author

I'll look into it at some point today 👍

@nojaf
Copy link
Member

nojaf commented May 15, 2025

Let's merge this one and open a follow-up issue?

@cometkim
Copy link
Member

VSCode extension finally supports ESM. I just noticed it.

If it needs a huge refactoring, let me know 😉

@nojaf
Copy link
Member

nojaf commented May 15, 2025

It might be a great time to revisit various aspects of the LSP server and extension. Let's create a list and develop a plan.

@zth
Copy link
Member

zth commented May 15, 2025

Let's merge this one and open a follow-up issue?

Yeah, we can merge this first for sure.

@zth zth merged commit f4ba829 into rescript-lang:master May 15, 2025
6 checks passed
@mediremi
Copy link
Member Author

Here's my attempt at an async rewrite: #1093

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants