Skip to content

Conversation

cknitt
Copy link
Member

@cknitt cknitt commented Aug 24, 2025

This builds on @cometkim's work in #7483 to extract the runtime files from the main rescript package into a new package @rescript/runtime, resolving a part of #6183.

Removal of @rescript/std shall be done in a separate PR.

@cknitt cknitt force-pushed the runtime-package branch 2 times, most recently from 9a5d72b to 4d7b90d Compare August 24, 2025 10:17
Copy link

pkg-pr-new bot commented Aug 24, 2025

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@7796

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@7796

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@7796

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@7796

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@7796

@rescript/runtime

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/runtime@7796

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@7796

commit: 5f4e6aa

@cknitt cknitt marked this pull request as ready for review August 24, 2025 12:08
@cknitt cknitt requested a review from cristianoc August 24, 2025 12:09
@cknitt cknitt requested a review from zth August 24, 2025 12:09
@nojaf
Copy link
Member

nojaf commented Aug 25, 2025

So, how would this work? I install rescript, and @rescript/runtime is installed as well?
Initial compile will do the one time compilation of the runtime?
A rescript clean never cleans the runtime?

I was expecting a package.json in packages/@rescript/runtime, don't we need that?

@cknitt
Copy link
Member Author

cknitt commented Aug 25, 2025

So, how would this work? I install rescript, and @rescript/runtime is installed as well?

Yes, it's installed automatically as a dependency of the rescript package, like the bin package for the platform.

Initial compile will do the one time compilation of the runtime? A rescript clean never cleans the runtime?

No, it is precompiled like it was before, nothing changed there. Only the artifacts (lib/ocaml, lib/es6, lib/js) are moved from the rescript package to the new @rescript/runtime package.

I was expecting a package.json in packages/@rescript/runtime, don't we need that?

It's there: https://github.com/rescript-lang/rescript/blob/runtime-package/packages/%40rescript/runtime/package.json

@nojaf
Copy link
Member

nojaf commented Aug 25, 2025

I see, this is more of a first step in a larger plan.

@mediremi
Copy link
Member

Will not including res(i) files in @rescript/runtime break "go to definition" for stdlib functions?

@cknitt
Copy link
Member Author

cknitt commented Aug 26, 2025

Will not including res(i) files in @rescript/runtime break "go to definition" for stdlib functions?

Maybe the commit message was not good, the last commit just prevents them from being included in the package root in addition to lib/ocaml. They are not really required in the package root. "Go to definition" jumps to the source files in lib/ocaml.

@@ -142,7 +142,7 @@ let findProjectFiles ~public ~namespace ~path ~sourceDirectories ~libBs =
in
let files =
dirs |> StringSet.elements
|> List.map (fun name -> Files.collect name isSourceFile)
|> List.map (fun name -> Files.collect ~maxDepth:2 name isSourceFile)
Copy link
Member

Choose a reason for hiding this comment

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

Curious, why is a maxDepth needed after this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

This part is from @cometkim.

From what I have seen, infinite recursion can occur during module resolution without it because rescript depends on @rescript/runtime, but @rescript/runtime also has rescript as a dev dependency.

Maybe we should add a comment there.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah a comment would be good.

Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

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

Just a minor Q: is "lib/ocaml" something we want to rename here, or perhaps later to keep changes self contained.
Unless it's the correct name for some legacy reason or other tooling back comp.

@cknitt
Copy link
Member Author

cknitt commented Aug 26, 2025

Just a minor Q: is "lib/ocaml" something we want to rename here, or perhaps later to keep changes self contained. Unless it's the correct name for some legacy reason or other tooling back comp.

Would be nice to change the name of this folder, but I think this is outside the scope of this PR and will also most probably break some tooling.

@cknitt cknitt merged commit bfec8a2 into master Aug 26, 2025
48 of 49 checks passed
@cknitt cknitt deleted the runtime-package branch August 26, 2025 13:47
if Files.exists path then Some path
else if Filename.dirname startPath = startPath then None
else resolveNodeModulePath ~startPath:(Filename.dirname startPath) name
if name = "@rescript/runtime" then
Copy link
Member

Choose a reason for hiding this comment

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

I don't remember exactly, but this is incomplete code. I think this is incompatible with the external-stdlib feature already.

I tried rewriting it to avoid relying on ninja's special stdlib resolution rules, but I wasn't successful. The current resolution rules still heavily rely on the node_modules directory structure and the runtime package name, which could be broken by package managers' storage design.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the module resolution in analysis/tools definitely has issues and should be revisited.

It does not work with workspace dependencies as can be seen here and also in #7525.

/cc @zth

Copy link
Member

Choose a reason for hiding this comment

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

I don't know much about the module resolution. What's the problem? Is it problem we handle as we should elsewhere but not in analysis/tools?

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.

6 participants