Skip to content

Conversation

nojaf
Copy link
Member

@nojaf nojaf commented Sep 19, 2025

Using the new information found in rescript-lang/rescript#7889

@nojaf nojaf requested review from zth and mediremi September 19, 2025 12:52
@zth
Copy link
Member

zth commented Sep 25, 2025

This looks good to me! Only 2 things I'd want to make sure is checked is that:

  • It gracefully falls back to the old code path when the compiler info json does not exist (it seems to do that)
  • It doesn't try and read any files in the hot path (for each incremental compilation, not when setting the compilation up at the first incremental only)

And to me it looks like both of those are fulfilled, right?

@nojaf
Copy link
Member Author

nojaf commented Sep 25, 2025

Good questions:

  • In case of the runtime (which is on the hot path), findRuntimeCached is used and that discovery logic is only ever ran once. Including the file read.
  • For bsc.exe, it is only called when projectRootState is not available in openedFile so it will only read the json file there once.
  • The old code is still in place as a fallback.

@nojaf nojaf marked this pull request as ready for review September 25, 2025 07:06
@nojaf nojaf merged commit 9faaa30 into rescript-lang:master Sep 25, 2025
6 checks passed
);
const contents = await fsAsync.readFile(compilerInfo, "utf8");
const compileInfo = JSON.parse(contents);
if (compileInfo && compileInfo.runtime_path) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@hackwaly problem is here!
Sorry about this, copy paste error

Choose a reason for hiding this comment

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

Shall we release a hotfix for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

There should be a new prerelease for this, please let me know how it goes.

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.

3 participants