Skip to content

Conversation

@nojaf
Copy link
Member

@nojaf nojaf commented Dec 13, 2025

Refactor findProjectRootOfFile to use separate functions for file vs directory lookups, eliminating confusing boolean parameter and fixing the case where exact directory matches were incorrectly excluded.

This fixed #1155 for me on Windows.

…lookup logic

Refactor findProjectRootOfFile to use separate functions for file vs directory lookups, eliminating confusing boolean parameter and fixing the case where exact directory matches were incorrectly excluded.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a Windows lockfile detection issue by refactoring the findProjectRootOfFile function to eliminate a confusing boolean parameter. The refactoring splits the functionality into two separate functions: one for file lookups (findProjectRootOfFile) and one for directory lookups (findProjectRootOfDir), with the key difference being that directory lookups allow exact matches while file lookups exclude them.

Key Changes:

  • Separated file and directory lookup logic into distinct functions with clear semantics
  • Removed the confusing allowDir boolean parameter from findProjectRootOfFile
  • Added comprehensive JSDoc documentation for the new function structure
  • Updated all call sites to use the appropriate function based on whether a file or directory is being searched

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
server/src/utils.ts Refactored findProjectRootOfFile into separate findProjectRootOfFile and findProjectRootOfDir functions with helper functions findProjectRootContainingFile and findProjectRootMatchingDir for clarity; added debug logging statements
server/src/incrementalCompilation.ts Updated to use new findProjectRootOfDir function for directory lookups

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@nojaf nojaf requested a review from zth December 13, 2025 20:21
Copy link
Member

@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 good from what I can tell!

@nojaf nojaf merged commit 441959d into rescript-lang:master Dec 13, 2025
6 checks passed
@cristianoc
Copy link
Collaborator

@nojaf this is great -- much cleaner
Is this root finding documented somewhere? I have the impression that we have now settled on precise rules to determine monorepo structure, so it might be possible to document how a monorepo should be set up.

@nojaf
Copy link
Member Author

nojaf commented Dec 14, 2025

Is this root finding documented somewhere? I have the impression that we have now settled on precise rules to determine monorepo structure, so it might be possible to document how a monorepo should be set up.

I would not say that we nailed down everything related to searching monorepo structures.
It is more like changes upon changes I think.
We have some detection but not on all levels:

  • Incremental compile will check for a lock file in a potential parent folder.
  • Runtime detection will take it into account as well.
  • But in findBinary parts of the fallback logic doesn't take it into account.

For the runtime and binary, in a best case a compiler-info.json file was found that gives an accurate result of where these things are.

We also don't quite take anything npm/pnpm/bun specific into account of a monorepo.
Not saying that we should, but it is something we don't do.

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.

The ReScript Language Server server crashed

3 participants