Resolve RescriptTools.binaryPath in user projects - #8694
cristianoc wants to merge 5 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 402ca04450
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "exports": { | ||
| "./lib/es6/*": "./lib/es6/*", | ||
| "./lib/js/*": "./lib/js/*", | ||
| "./cli/bins": "./cli/common/bins.js", |
There was a problem hiding this comment.
This changes the behavior of a published runtime API and fixes a user-visible failure, but the commit does not add the required entry under the current (Unreleased) bug-fix section of CHANGELOG.md. Add an entry ending with the PR link so the fix is included in the release notes.
AGENTS.md reference: AGENTS.md:L93-L95
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Added in d46ee1d, under the Unreleased section with the PR link.
| ``` | ||
| */ | ||
| @module("#cli/bins") | ||
| @module("rescript/cli/bins") |
There was a problem hiding this comment.
When a consumer uses the supported package-specs.module: "commonjs" output, this external is emitted as require("rescript/cli/bins"). That export resolves to cli/common/bins.js, which is an ES module containing top-level await, so Node throws ERR_REQUIRE_ASYNC_MODULE; the added test only exercises esmodule output and misses this case. Provide a CommonJS-compatible/conditional entry point or otherwise avoid synchronously requiring the top-level-await module.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed: with module: commonjs, require("rescript/cli/bins") loaded cli/common/bins.js, and its top-level await made Node throw ERR_REQUIRE_ASYNC_MODULE. 629d9dd adds cli/common/bins.cjs, which finds the platform package with require.resolve and builds the same paths synchronously. The ./cli/bins export now sends the require condition there and keeps bins.js as the default for imports. tests/build_tests/rescript_tools_binary_path now emits both ES module (.mjs) and CommonJS (.cjs) output and checks that both print the tools binary path. Before the fix the CommonJS run fails with ERR_REQUIRE_ASYNC_MODULE.
There was a problem hiding this comment.
Follow-up in 3af68e8: cli/common/bins.cjs is now the only implementation of the platform lookup, and bins.js re-exports the same names from it, so the ESM and CommonJS entries cannot drift. The exported names and values are identical to before, and rescript_tools_binary_path still passes for both outputs.
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
|
Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-8694 |
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
The Could we instead add a small |
RescriptTools.binaryPath was `@module("#cli/bins")`. An external is
emitted at its use site, so the `import ... from "#cli/bins"` lands in
the user's compiled module, and Node resolves a `#` specifier against
the "imports" field of the package.json closest to that module. Only
the rescript package defines `#cli/*`, so in a user project the import
fails with ERR_PACKAGE_IMPORT_NOT_DEFINED. A scratch project linking
the workspace's rescript, @rescript/runtime and @rescript/darwin-arm64
packages reproduced this.
The rescript package now exports "./cli/bins" (cli/common/bins.js) and
binaryPath imports "rescript/cli/bins", which resolves from any
project that depends on rescript.
tests/build_tests/rescript_tools_binary_path compiles a module that
prints RescriptTools.binaryPath into a src/ with its own package.json,
links node_modules/rescript to the repository and runs it. It fails
with ERR_PACKAGE_IMPORT_NOT_DEFINED before this change and prints the
rescript-tools path after it; node scripts/test.js -build passes.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Cristiano Calcagno <cristianoc@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Cristiano Calcagno <cristianoc@users.noreply.github.com>
CommonJS output compiles RescriptTools.binaryPath to
require("rescript/cli/bins"). cli/common/bins.js uses top-level await,
so require fails with ERR_REQUIRE_ASYNC_MODULE. The export now maps the
require condition to cli/common/bins.cjs, which resolves the platform
package synchronously, and keeps bins.js for imports. The build test
emits both ES module and CommonJS output and runs each.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Cristiano Calcagno <cristianoc@users.noreply.github.com>
bins.js re-exports the paths from bins.cjs, so ES module and CommonJS consumers share one platform lookup. bins.cjs now also reports an unsupported Node.js version when the platform package is missing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Cristiano Calcagno <cristianoc@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Cristiano Calcagno <cristianoc@users.noreply.github.com>
d01a4f5 to
4108171
Compare
|
@cknitt Done in 4108171: |
|
@codex review |
This branch has not been deployed
2 participants
Closes #8712.
Summary
@module("#cli/bins"). An external is emitted at its use site, so theimport ... from "#cli/bins"lands in the user's compiled module, and Node resolves a#specifier against the "imports" field of the package.json closest to that module. Only the rescript package defines#cli/*, so in a user project the import fails with ERR_PACKAGE_IMPORT_NOT_DEFINED. A scratch project linking the workspace's rescript, @rescript/runtime and @rescript/darwin-arm64 packages reproduced this.Test plan
dune builddune build @fmtdune build --profile browser @checkmakemake liblib/outputsmake testyarn apidocs:generatemake playgroundyarn workspace playground testmake checkformatnpm run check🤖 Generated with Claude Code