Skip to content
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

Use fs api to check if string is a directory. #1080

Merged
merged 3 commits into from
Mar 28, 2025

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Mar 28, 2025

I found out why the incremental build wasn't working yesterday.
Turns out I have a dot in my project folder name, this was mistaken for a file.
That file is not part of the project, thus the rescript version could not be found for the root.

@nojaf
Copy link
Contributor Author

nojaf commented Mar 28, 2025

Ping @zth

Comment on lines 70 to 71
const isDir = path.extname(source) === "";
const dirStat = fs.statSync(source);
const isDir = dirStat.isDirectory();
Copy link
Collaborator

Choose a reason for hiding this comment

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

IIRC, this is in a hot path where we shouldn't use fs functions (which is why this fn looks the way it looks now). So we need to figure out another way to do this, outside of the hot path. OTOH maybe it could be aggressively cached in some sane way (unlikely), or moved outside of the hot path entirely to some sort of setup function somewhere. Would need to be explored.

@@ -268,7 +268,7 @@ let openedFile = (fileUri: string, fileContent: string) => {
filesDiagnostics: {},
namespaceName:
namespaceName.kind === "success" ? namespaceName.result : null,
rescriptVersion: utils.findReScriptVersion(projectRootPath),
rescriptVersion: utils.findReScriptVersionForProjectRoot(projectRootPath),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the root of the problem really.
Here my project is called ronnies.be, which is mistaken for a file.
At this point, we know it is a folder, so we can shortcut things I think.

@zth
Copy link
Collaborator

zth commented Mar 28, 2025

@nojaf ready for a new review...?

@nojaf
Copy link
Contributor Author

nojaf commented Mar 28, 2025

@zth yup, go for it

Copy link
Collaborator

@zth zth left a comment

Choose a reason for hiding this comment

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

Looks fine to me! Although I have not tested it.

@zth zth merged commit cd42e8b into rescript-lang:master Mar 28, 2025
6 checks passed
@nojaf
Copy link
Contributor Author

nojaf commented Mar 28, 2025

Works like a charm in the prerelease channel!

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.

2 participants