Skip to content

Conversation

@nojaf
Copy link
Member

@nojaf nojaf commented Dec 18, 2025

In a project with a namespace, I saw in incrementalCompilationFileChanged the following:

// client reports changed cmt file 
file:///Users/nojaf/Projects/rescript-kaplay/packages/skirmish/lib/bs/src/Healthbar-Skirmish.cmt

// In incrementalCompilationFileChanged we receive normalized file:
/Users/nojaf/Projects/rescript-kaplay/packages/skirmish/lib/bs/src/Healthbar-Skirmish.cmt,

// `originalTypeFileToFilePath` contained:

map: {"/Users/nojaf/Projects/rescript-kaplay/packages/skirmish/lib/bs/src/Healthbar.cmt":"/Users/nojaf/Projects/rescript-kaplay/packages/skirmish/src/Healthbar.res"}

// Notice key is missing namespace so we get:
filePath is undefined

To fix this, I'm taking the namespace into account during incrementalFileCacheEntry entry.

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 bug in incremental compilation where the namespace was not being considered when constructing the original type file location. This caused file lookups to fail in projects with namespaces, as the generated .cmt/.cmti files include the namespace suffix but the lookup key did not.

Key Changes:

  • Refactored the construction of originalTypeFileLocation to explicitly build the filename with namespace consideration
  • Replaced path manipulation logic (path.parse/path.format) with direct filename construction
  • Added proper namespace suffix to type file names when project.namespaceName is present

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

@nojaf nojaf requested a review from zth December 18, 2025 08:34
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.

Ah, nice find!

@nojaf nojaf merged commit d216af4 into rescript-lang:master Dec 18, 2025
6 checks passed
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