- Notifications
You must be signed in to change notification settings - Fork471
@rescript/runtime package#7796
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
# Conflicts:#cli/common/bsb.js
9a5d72b to4d7b90dComparepkg-pr-newbot commentedAug 24, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
rescript@rescript/darwin-arm64@rescript/darwin-x64@rescript/linux-arm64@rescript/linux-x64@rescript/runtime@rescript/win32-x64commit: |
So, how would this work? I install rescript, and I was expecting a |
Yes, it's installed automatically as a dependency of the
No, it is precompiled like it was before, nothing changed there. Only the artifacts (lib/ocaml, lib/es6, lib/js) are moved from the
|
I see, this is more of a first step in a larger plan. |
Will not including |
Maybe the commit message was not good, the last commit just prevents them from being included in the package rootin addition to |
| let files= | ||
| dirs|>StringSet.elements | ||
| |>List.map (funname ->Files.collect name isSourceFile) | ||
| |>List.map (funname ->Files.collect~maxDepth:2name isSourceFile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Curious, why is amaxDepth needed after this PR?
There was a problem hiding this comment.
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 becauserescript depends on@rescript/runtime, but@rescript/runtime also hasrescript as a dev dependency.
Maybe we should add a comment there.
There was a problem hiding this comment.
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.
cristianoc left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this 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.
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. |
bfec8a2 intomasterUh oh!
There was an error while loading.Please reload this page.
| ifFiles.exists paththenSome path | ||
| elseifFilename.dirname startPath= startPaththenNone | ||
| else resolveNodeModulePath~startPath:(Filename.dirname startPath) name | ||
| if name="@rescript/runtime"then |
There was a problem hiding this comment.
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 theexternal-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 thenode_modules directory structure and the runtime package name, which could be broken by package managers' storage design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
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?
Uh oh!
There was an error while loading.Please reload this page.
This builds on@cometkim's work in#7483 to extract the runtime files from the main
rescriptpackage into a new package@rescript/runtime, resolving a part of#6183.Removal of
@rescript/stdshall be done in a separate PR.