Fix ReScript indexers without shamefully hoisting deps + removes deps on user's start script #580
Conversation
WalkthroughThis update systematically replaces references to BuckleScript output files ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI Script
participant File System
participant Child Process
User->>CLI Script: Run Index.bs.js migration script
CLI Script->>File System: Search for package.json upwards
File System-->>CLI Script: Return path to package.json
CLI Script->>File System: Read and parse package.json
CLI Script->>File System: Update start script to use 'envio start'
CLI Script->>File System: Write updated package.json
CLI Script->>Child Process: Spawn 'envio start'
Child Process-->>CLI Script: Forward stdout/stderr, exit code
CLI Script-->>User: Log status and exit
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The PR has two breaking changes:
|
|
Related to #568 |
| {{#if is_typescript}} | ||
| "start": "ts-node generated/src/Index.bs.js" | ||
| "start": "ts-node generated/src/Index.res.js" | ||
| {{else}} | ||
| "start": "node generated/src/Index.bs.js" |
There was a problem hiding this comment.
This will break for existing indexers
|
Note: The PR is still draft, I need to include |
| @@ -0,0 +1,135 @@ | |||
| #!/usr/bin/env node | |||
There was a problem hiding this comment.
Should look like this, but for some reason it's not included to the generated directory
There was a problem hiding this comment.
Ok, looks like it's working correctly now
There was a problem hiding this comment.
When does this script get run?
Should it always be logging this migration log?
There was a problem hiding this comment.
Why is this file ".bs.js"? Surely it should just be Index.js?
|
The main problem here was that the rescript project doesn't build I should have also install all dependencies correctly, as well as update the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
codegenerator/cli/templates/dynamic/init_templates/shared/package.json.hbs (1)
19-19: This breaking change aligns with the migration goals but requires user action.The simplified start script using
envio startinstead of direct file execution is part of the broader migration from.bs.jsto.res.jsnaming. This change will require existing users to update their workflows.
🧹 Nitpick comments (8)
codegenerator/cli/src/project_paths/handler_paths.rs (1)
12-12: Exposerelative_to_configas public for external access
Therelative_to_configfield is now accessible outside this module, which aligns with its intended use in codegen templates. Consider whether the other path fields (relative_to_generated_src,relative_to_project_root) should also be exposed via getters or made public for API consistency.codegenerator/cli/templates/static/blank_template/shared/.gitignore (1)
33-34: Add.res.jsand.res.mjsignore patterns
These entries ensure generated ReScript files are excluded from version control. For conciseness, you could merge them into a single glob pattern (e.g.,*.res.{js,mjs}).scenarios/helpers/.gitignore (1)
8-8: Ignore new ReScript module format
Adding*.res.mjsensures generated ES modules are excluded from version control, consistent with.res.jsignores.Consider removing
*.bs.jsif BuckleScript outputs are no longer generated.scenarios/fuel_test/.gitignore (1)
32-33: Clean up legacy ignore patternsConsider removing
*.bs.jsnow that only.res.jsis generated. Dropping obsolete patterns avoids clutter and potential confusion.codegenerator/cli/templates/static/erc20_template/rescript/config.yaml (1)
10-10: Standardize handler path syntaxThe other template uses
./src/.... For consistency, prepend./here as well:-handler: src/EventHandlers.res.js +handler: ./src/EventHandlers.res.jscodegenerator/integration_tests/tests/test_indexers/test_exits/package.json (1)
9-9: Consider usingnodeinstead ofts-nodefor.res.jsartifacts
Since this entrypoint is a plain JS file generated by ReScript, running it withnode generated/src/Index.res.jsavoids pulling in the TypeScript runtime and may simplify dependencies.scenarios/test_codegen/.gitignore (1)
31-32: Remove obsolete.bs.jspatterns if fully migrated
You’ve added*.res.jsand*.res.mjsbut still list*.bs.js/*.bs.mjsbelow. Dropping the old patterns will prevent accidentally committing stale BuckleScript artifacts.codegenerator/integration_tests/tests/test_indexers/dynamic_contracts/package.json (1)
12-12: Point start script to ReScript entrypointThe
startscript now runsIndex.res.jsin the generated output, reflecting the suffix migration.Consider using
node generated/src/Index.res.jsinstead ofts-nodefor pure JavaScript files to simplify execution.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
scenarios/test_codegen/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (61)
codegenerator/cli/npm/envio/.gitignore(1 hunks)codegenerator/cli/npm/envio/index.js(2 hunks)codegenerator/cli/npm/envio/package.json(1 hunks)codegenerator/cli/npm/envio/package.json.tmpl(2 hunks)codegenerator/cli/npm/envio/rescript.json(1 hunks)codegenerator/cli/npm/envio/src/bindings/BigDecimal.gen.ts(1 hunks)codegenerator/cli/npm/envio/src/bindings/Ethers.gen.ts(1 hunks)codegenerator/cli/src/cli_args/init_config.rs(1 hunks)codegenerator/cli/src/commands.rs(1 hunks)codegenerator/cli/src/config_parsing/graph_migration/mod.rs(2 hunks)codegenerator/cli/src/hbs_templating/codegen_templates.rs(2 hunks)codegenerator/cli/src/project_paths/handler_paths.rs(1 hunks)codegenerator/cli/templates/dynamic/codegen/package.json.hbs(2 hunks)codegenerator/cli/templates/dynamic/init_templates/shared/package.json.hbs(2 hunks)codegenerator/cli/templates/dynamic/subgraph_migration_templates/javascript/src/EventHandlers.js.hbs(1 hunks)codegenerator/cli/templates/static/blank_template/rescript/rescript.json(1 hunks)codegenerator/cli/templates/static/blank_template/shared/.gitignore(1 hunks)codegenerator/cli/templates/static/codegen/.gitignore(1 hunks)codegenerator/cli/templates/static/codegen/index.js(1 hunks)codegenerator/cli/templates/static/codegen/rescript.json(1 hunks)codegenerator/cli/templates/static/codegen/src/Config.res(1 hunks)codegenerator/cli/templates/static/codegen/src/Index.bs.js(1 hunks)codegenerator/cli/templates/static/codegen/src/Index.res(0 hunks)codegenerator/cli/templates/static/codegen/src/db/DbFunctionsImplementation.js(18 hunks)codegenerator/cli/templates/static/erc20_template/rescript/config.yaml(1 hunks)codegenerator/cli/templates/static/erc20_template/rescript/rescript.json(1 hunks)codegenerator/cli/templates/static/erc20_template/shared/.gitignore(1 hunks)codegenerator/cli/templates/static/greeter_template/rescript/README.md(1 hunks)codegenerator/cli/templates/static/greeter_template/rescript/config.yaml(1 hunks)codegenerator/cli/templates/static/greeter_template/rescript/rescript.json(1 hunks)codegenerator/cli/templates/static/greeter_template/shared/.gitignore(1 hunks)codegenerator/cli/templates/static/greeteronfuel_template/rescript/config.yaml(1 hunks)codegenerator/cli/templates/static/greeteronfuel_template/rescript/rescript.json(1 hunks)codegenerator/cli/templates/static/greeteronfuel_template/shared/.gitignore(1 hunks)codegenerator/integration_tests/tests/test_indexers/dynamic_contracts/.gitignore(1 hunks)codegenerator/integration_tests/tests/test_indexers/dynamic_contracts/package.json(1 hunks)codegenerator/integration_tests/tests/test_indexers/test_exits/.gitignore(1 hunks)codegenerator/integration_tests/tests/test_indexers/test_exits/package.json(1 hunks)codegenerator/integration_tests/tests/test_indexers/wildcard-uni-factory/.gitignore(1 hunks)codegenerator/integration_tests/tests/test_indexers/wildcard-uni-factory/package.json(1 hunks)scenarios/erc20_multichain_factory/.gitignore(1 hunks)scenarios/erc20_multichain_factory/.mocharc.json(1 hunks)scenarios/erc20_multichain_factory/config.yaml(1 hunks)scenarios/erc20_multichain_factory/package.json(1 hunks)scenarios/erc20_multichain_factory/rescript.json(1 hunks)scenarios/erc20_multichain_factory/test/DbHelpers.res(1 hunks)scenarios/fuel_test/.gitignore(1 hunks)scenarios/fuel_test/package.json(1 hunks)scenarios/fuel_test/rescript.json(1 hunks)scenarios/helpers/.gitignore(1 hunks)scenarios/test_codegen/.gitignore(1 hunks)scenarios/test_codegen/.mocharc.json(1 hunks)scenarios/test_codegen/config.yaml(2 hunks)scenarios/test_codegen/package.json(2 hunks)scenarios/test_codegen/rescript.json(1 hunks)scenarios/test_codegen/test/Integration_ts_helpers.gen.ts(1 hunks)scenarios/test_codegen/test/helpers/DbHelpers.res(1 hunks)scenarios/test_codegen/test/helpers/utils.ts(1 hunks)scenarios/test_codegen/test/integration-raw-events-test.ts(2 hunks)scenarios/test_codegen/test/manual/README.md(1 hunks)scenarios/test_codegen/test/topic-hashing-test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- codegenerator/cli/templates/static/codegen/src/Index.res
🧰 Additional context used
🧠 Learnings (2)
scenarios/test_codegen/test/helpers/DbHelpers.res (2)
Learnt from: JonoPrest
PR: enviodev/hyperindex#555
File: scenarios/test_codegen/test/lib_tests/EntityHistory_test.res:582-629
Timestamp: 2025-05-27T17:07:04.699Z
Learning: In ReScript test files using DbHelpers.resetPostgresClient(), this function is synchronous with signature `unit => unit` and should not be awaited. It performs a raw JavaScript reassignment of the postgres client and does not return a promise.
Learnt from: JonoPrest
PR: enviodev/hyperindex#555
File: scenarios/test_codegen/test/lib_tests/EntityHistory_test.res:582-629
Timestamp: 2025-05-27T17:07:04.699Z
Learning: In ReScript test files using DbHelpers.resetPostgresClient(), this function is synchronous with signature `unit => unit` and should not be awaited. It performs a raw JavaScript reassignment of the postgres client and does not return a promise.
scenarios/erc20_multichain_factory/test/DbHelpers.res (2)
Learnt from: JonoPrest
PR: enviodev/hyperindex#555
File: scenarios/test_codegen/test/lib_tests/EntityHistory_test.res:582-629
Timestamp: 2025-05-27T17:07:04.699Z
Learning: In ReScript test files using DbHelpers.resetPostgresClient(), this function is synchronous with signature `unit => unit` and should not be awaited. It performs a raw JavaScript reassignment of the postgres client and does not return a promise.
Learnt from: JonoPrest
PR: enviodev/hyperindex#555
File: scenarios/test_codegen/test/lib_tests/EntityHistory_test.res:582-629
Timestamp: 2025-05-27T17:07:04.699Z
Learning: In ReScript test files using DbHelpers.resetPostgresClient(), this function is synchronous with signature `unit => unit` and should not be awaited. It performs a raw JavaScript reassignment of the postgres client and does not return a promise.
🪛 Biome (1.9.4)
scenarios/test_codegen/.mocharc.json
[error] 5-5: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 5-5: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 5-5: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 5-5: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_and_test
- GitHub Check: build_and_test
🔇 Additional comments (66)
scenarios/fuel_test/rescript.json (1)
13-13: Update ReScript output suffix to.res.js
Aligns this scenario’s compiler configuration with the new file naming convention.codegenerator/cli/templates/static/blank_template/rescript/rescript.json (1)
21-21: Switch ReScript suffix to.res.js
Reflects the updated ReScript output naming in the blank template’s configuration.codegenerator/cli/templates/static/greeter_template/rescript/rescript.json (1)
21-21: Switch Greeter template suffix to.res.js
Ensures the Greeter template uses the new ReScript output file extension.scenarios/test_codegen/test/topic-hashing-test.ts (2)
1-1: Update TestEvents import to ReScript output file
Import path suffix updated from.bs.jsto.res.jscorrectly aligns with the new ReScript conventions.
4-4: Update Rpc import to ReScript suffix
TheGetLogsimport path has been updated to.res.js, matching the ReScript migration.codegenerator/cli/templates/static/erc20_template/rescript/rescript.json (1)
21-21: Align compiled file suffix with ReScript
The"suffix"field has been updated from.bs.jsto.res.jsin the template, ensuring generated artifacts follow ReScript naming conventions.codegenerator/cli/templates/dynamic/subgraph_migration_templates/javascript/src/EventHandlers.js.hbs (1)
9-9: Update handler import to.res.js
The require path forHandlershas been changed to.res.js, consistent with the overall migration.scenarios/erc20_multichain_factory/.mocharc.json (1)
2-2: Update test spec pattern to ReScript outputs
The Mocha"spec"pattern now targets**/*.res.jsfiles, reflecting the new output suffix.scenarios/test_codegen/rescript.json (1)
14-14: Change output suffix to ReScript convention
The"suffix"property has been updated to.res.js, aligning the test scenario with the ReScript migration.scenarios/test_codegen/test/integration-raw-events-test.ts (1)
5-6: Consistent path updates for ReScript output
Imports have been correctly updated to use.res.jsextensions, aligning with the new naming convention.scenarios/erc20_multichain_factory/rescript.json (1)
32-32: Update ReScript suffix to.res.js
Thesuffixfield has been changed to.res.js, matching the migration from BuckleScript outputs. Ensure this aligns with your build and tooling configurations.codegenerator/cli/templates/static/greeteronfuel_template/rescript/rescript.json (1)
21-21: Align template output suffix with ReScript convention
Thesuffixis now.res.js, consistent with the rest of the project’s migration away from.bs.js.scenarios/erc20_multichain_factory/package.json (1)
11-11: Switch start script to.res.jsentry point
Thestartscript now points toIndex.res.js, reflecting the new build output.codegenerator/integration_tests/tests/test_indexers/wildcard-uni-factory/package.json (1)
12-12: Update start script to .res.js: ApprovedThe
startscript now correctly targets the ReScript-generatedIndex.res.jsentry point. Ensure your test harness generates this file before running the script.scenarios/fuel_test/package.json (1)
16-16: Align start script with ReScript output: ApprovedThe
startcommand now invokesgenerated/src/Index.res.jsas expected. Confirm that your build step (res:build) precedes this invocation in CI or local workflows.codegenerator/cli/templates/static/greeter_template/rescript/config.yaml (1)
10-10: Handler path updated correctlyThe contract’s
handlernow points toEventHandlers.res.js, matching the ReScript output convention.scenarios/test_codegen/test/helpers/utils.ts (1)
1-1: ReScript imports updated correctly to.res.js
TherunUpMigrationsandconfigimports now point to.res.jsoutputs as expected.Also applies to: 3-3
codegenerator/integration_tests/tests/test_indexers/dynamic_contracts/.gitignore (1)
33-34: Ignore new ReScript output suffixes
Adding*.res.jsand*.res.mjsensures these generated files are excluded from source control in this test scenario.scenarios/erc20_multichain_factory/config.yaml (1)
13-13: Correctly updated event handler suffix
The handler path is now.res.js, aligning with ReScript naming conventions. Ensure the correspondingEventHandlers.res.jsfile is present undersrc/.codegenerator/cli/npm/envio/rescript.json (1)
10-10: Updated ReScript output suffix
Thesuffixfield has been changed to.res.js, matching the new compiler output. Confirm that no leftover.bs.jsartifacts remain in the build outputs.scenarios/test_codegen/test/helpers/DbHelpers.res (1)
6-6: Renamed raw require path
The raw JS require now points toDb.res.jsinstead ofDb.bs.js. SinceresetPostgresClientis synchronous (unit => unit), noawaitis needed here.codegenerator/cli/npm/envio/.gitignore (1)
137-138: Ignore new ReScript artifacts
Added*.res.jsand*.res.mjsto ensure generated ReScript outputs are excluded from version control.codegenerator/cli/templates/static/codegen/.gitignore (1)
29-30: Update ignore patterns for ReScript outputs
Switching to*.res.js/*.res.mjsaligns the template’s.gitignorewith the updated file suffixes.scenarios/test_codegen/.mocharc.json (1)
5-5: Update test spec pattern to new ReScript suffixThe Mocha spec now includes
.res.jsoutput files, aligning with the ReScript compilation convention.🧰 Tools
🪛 Biome (1.9.4)
[error] 5-5: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 5-5: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 5-5: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 5-5: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
codegenerator/cli/templates/static/greeteronfuel_template/rescript/config.yaml (1)
11-11: Use ReScript handler file in templateThe
handlerpath now referencesEventHandlers.res.js, matching the updated ReScript output.codegenerator/cli/src/cli_args/init_config.rs (1)
343-343: Align event handler path for ReScriptThe
Language::ReScriptbranch now returns the correct.res.jshandler path.scenarios/erc20_multichain_factory/.gitignore (1)
32-33: Ignore ReScript compiled outputsAdd patterns for
.res.jsand.res.mjsto avoid committing generated files.scenarios/test_codegen/test/manual/README.md (1)
11-21: Update.bs.jsreferences to.res.jsin manual tests.The example commands now correctly reference
LogTesting.res.jsfor all log strategies.codegenerator/cli/npm/envio/src/bindings/BigDecimal.gen.ts (1)
6-6: Require correct ReScript output file.Updated
require('./BigDecimal.res.js')aligns with the new naming convention.scenarios/test_codegen/test/Integration_ts_helpers.gen.ts (1)
6-6: Require correct ReScript output file.Updated
require('./Integration_ts_helpers.res.js')to match the ReScript output changes.codegenerator/integration_tests/tests/test_indexers/wildcard-uni-factory/.gitignore (1)
32-33: Add new ReScript artifact patterns to ignore.Added
*.res.jsand*.res.mjsto ignore the updated ReScript outputs.codegenerator/cli/templates/static/greeteronfuel_template/shared/.gitignore (1)
32-33: Add new ReScript artifact patterns to ignore.Added
*.res.jsand*.res.mjsto keep template projects in sync with ReScript output.codegenerator/cli/templates/static/greeter_template/shared/.gitignore (1)
32-33: Update ignore patterns for ReScript outputs
Ignoring the new.res.jsand.res.mjsartifacts aligns with the migration from BuckleScript to ReScript.codegenerator/cli/templates/static/codegen/rescript.json (1)
27-27: Switch generated file suffix to.res.js
Thesuffixfield now matches the new ReScript output convention, ensuring generated files use the correct extension.codegenerator/cli/templates/static/erc20_template/shared/.gitignore (1)
32-33: Update ignore patterns for ReScript outputs
Aligned the ERC-20 template’s.gitignorewith the new.res.jsand.res.mjssuffixes.codegenerator/cli/templates/static/codegen/index.js (1)
5-6: Adjust imports to use.res.jsextensions
Module require paths have been updated to reflect the ReScript build outputs instead of BuckleScript.codegenerator/integration_tests/tests/test_indexers/test_exits/.gitignore (1)
33-34: Update ignore patterns for ReScript outputs
Ensure.res.jsand.res.mjsfiles are correctly excluded in the integration test environment.codegenerator/cli/src/config_parsing/graph_migration/mod.rs (2)
117-117: LGTM: ReScript file extension migration applied correctly.The update from
.bs.jsto.res.jsaligns with the broader migration from BuckleScript to ReScript naming conventions.
557-557: Test updated correctly to match implementation.The test expectation properly reflects the new ReScript file extension, ensuring consistency between the implementation and tests.
codegenerator/cli/templates/static/codegen/src/Config.res (1)
103-103: Enhanced error formatting for better debugging.The addition of
Internal.prettifyExnimproves error message formatting when Hasura table tracking fails, making debugging easier.scenarios/test_codegen/package.json (1)
14-14: NPM scripts updated for ReScript file extension migration.Both scripts correctly reference the new
.res.jsoutput files, ensuring compatibility with the ReScript naming convention migration.Also applies to: 23-23
codegenerator/cli/src/commands.rs (1)
178-178:✅ Verification successful
Verify working directory change for npm start command.
The working directory for the
npm run startcommand has been changed fromproject_paths.project_roottoproject_paths.generated. Ensure this change is compatible with the package.json scripts and that the start command works correctly from the generated directory.
🏁 Script executed:
#!/bin/bash # Description: Verify that package.json files in generated directories contain the correct start scripts # Search for package.json files that might be in generated directories fd -t f "package.json" | xargs grep -l "start.*Index\.res\.js" # Check if there are any references to the old .bs.js files in package.json files rg -A 2 -B 2 "\.bs\.js" --type jsonLength of output: 523
I’d like to inspect the surrounding code and the package.json template to confirm the start command is wired up correctly in the generated directory.
🏁 Script executed:
#!/bin/bash # Show commands.rs around the invocation echo "=== commands.rs (lines 150–210) ===" sed -n '150,210p' codegenerator/cli/src/commands.rs # Show the package.json.hbs used in the generated folder echo "" echo "=== codegenerator/cli/templates/dynamic/codegen/package.json.hbs ===" sed -n '1,200p' codegenerator/cli/templates/dynamic/codegen/package.json.hbsLength of output: 4245
Confirmed: Working directory for
npm run startis correctThe generated directory includes a
package.json(from the provided Handlebars template) with a"start": "node src/Index.res.js"script, so runningnpm run startinproject_paths.generatedis intentional and valid.codegenerator/cli/templates/static/greeter_template/rescript/README.md (2)
27-27: LGTM! Handler path correctly updated for ReScript migration.The change from
.bs.jsto.res.jsaligns with the broader migration from BuckleScript to ReScript output conventions, ensuring users get the correct configuration for new projects.
29-30: Good cleanup of trailing whitespace.Removing trailing spaces improves formatting consistency.
codegenerator/cli/npm/envio/package.json.tmpl (2)
39-40: LGTM! Logging dependencies added.The addition of
pinoandpino-prettydependencies suggests enhanced logging capabilities for the envio package.
49-49: Good simplification of files array.Removing the
.bs.jsexclusion pattern is appropriate since the migration moves to.res.jsfiles. The simplified approach of just including"src"is cleaner and more maintainable.codegenerator/cli/npm/envio/package.json (2)
28-29: LGTM! Consistent logging dependencies.The addition of
pinologging dependencies is consistent with the template file changes.
31-39: Appropriate explicit file inclusion list.The detailed files array provides precise control over package contents for development builds, including necessary binaries, schemas, and source files.
scenarios/erc20_multichain_factory/test/DbHelpers.res (1)
6-6: Essential path update for ReScript migration.The require path correctly changes from
.bs.jsto.res.jsto match the new ReScript output conventions. This update is necessary for theresetPostgresClientfunction to continue working after the migration.scenarios/test_codegen/config.yaml (1)
12-12: LGTM: Handler paths correctly updated for ReScript migration.The configuration file has been properly updated to reference
.res.jsfiles instead of.bs.jsfiles, which aligns with the migration from BuckleScript to ReScript naming conventions.Also applies to: 16-16, 37-37
codegenerator/cli/src/hbs_templating/codegen_templates.rs (3)
1231-1231: LGTM: TypeScript detection field added correctly.The new
has_typescriptboolean field is properly added to theProjectTemplatestruct to support conditional logic in templates.
1335-1337: LGTM: TypeScript detection logic is sound.The implementation correctly determines if TypeScript is used by checking if any contract handler path ends with ".ts". This is a simple and effective approach for template conditionals.
1341-1341: LGTM: Field properly assigned in constructor.The
has_typescriptfield is correctly assigned in theProjectTemplateconstructor, ensuring the detection result is available to templates.codegenerator/cli/npm/envio/index.js (1)
5-5: LGTM: Import paths correctly updated for ReScript migration.The module import paths have been properly updated from
.bs.jsto.res.jsextensions, maintaining consistency with the broader migration from BuckleScript to ReScript naming conventions.Also applies to: 33-33
codegenerator/cli/templates/dynamic/codegen/package.json.hbs (3)
12-15: LGTM: Script paths correctly updated for ReScript migration.All database and benchmark scripts have been properly updated to reference
.res.jsfiles instead of.bs.jsfiles, maintaining consistency with the ReScript migration.
16-16: LGTM: Smart conditional TypeScript support implemented.The start script correctly uses the
has_typescripttemplate variable to conditionally prependts-to the node command, enabling seamless TypeScript execution when needed. This integrates well with the TypeScript detection logic in the Rust code.
60-60: LGTM: ts-node dependency added to support TypeScript execution.The addition of
ts-nodedependency enables the conditional TypeScript support in the start script, completing the TypeScript integration feature.codegenerator/cli/templates/dynamic/init_templates/shared/package.json.hbs (1)
44-44:❓ Verification inconclusive
Verify the Node.js version requirement increase.
The Node.js version requirement was increased from
>=18.0.0to>=20.6.0. Ensure this aligns with the actual requirements of theenvio startcommand and document this breaking change for users.
🏁 Script executed:
#!/bin/bash # Check what Node.js features are being used that require 20.6.0+ rg -A 5 -B 5 "20\.6\.0|node.*20"Length of output: 32441
🏁 Script executed:
#!/bin/bash set -eux # Search for Node-specific APIs that may require newer Node.js versions in the CLI implementation rg -n "global\.fetch" -A2 -B2 codegenerator/cli || true rg -n "AbortController" -A2 -B2 codegenerator/cli || true rg -n "from 'node:" -A2 -B2 codegenerator/cli || true rg -n "diagnostics_channel" -A2 -B2 codegenerator/cli || true rg -n "timers/promises" -A2 -B2 codegenerator/cli || true rg -n "AbortSignal.timeout" -A2 -B2 codegenerator/cli || trueLength of output: 361
Review Node.js engine bump
I didn’t find any Node 20.6+-specific APIs in the CLI code that would strictly require raising the minimum from ≥18.0.0 to ≥20.6.0. If this change is intentional (for policy, alignment with downstream engines, etc.), please:
- Document the breaking change in your release notes or migration guide.
- Confirm that users of
envio startunderstand they must upgrade Node.js.• Location:
codegenerator/cli/templates/dynamic/init_templates/shared/package.json.hbs
Line 44:"node": ">=20.6.0"(previously"node": ">=18.0.0")codegenerator/cli/templates/static/codegen/src/db/DbFunctionsImplementation.js (2)
1-3: Import path migration correctly implemented.The import paths have been correctly updated from
.bs.jsto.res.jsas part of the ReScript migration. This aligns with the broader codebase changes.
24-24: Formatting improvements enhance code consistency.The removal of trailing commas in template literals and function parameters improves code consistency and follows JavaScript best practices.
Also applies to: 60-60, 97-97, 129-129, 155-156, 159-159, 162-162, 171-171, 190-190, 204-204, 216-216, 229-229, 246-246, 278-278, 303-303, 321-321, 351-351, 384-384, 397-397, 399-399
codegenerator/cli/templates/static/codegen/src/Index.bs.js (6)
1-1: Clarify the .bs.js filename convention for this migration script.This file is named
Index.bs.jsbut the PR is migrating away from.bs.jsto.res.jsnaming. Is this intentional since it's a migration tool, or should it follow the new naming convention?
17-24: Directory traversal logic is correct and secure.The upward directory search correctly uses
path.dirname()and stops at the filesystem root. This prevents infinite loops and handles edge cases properly.
72-75: JSON formatting preserves structure correctly.The
JSON.stringifywith proper indentation and trailing newline maintains package.json formatting standards.
92-96: Process spawning is secure with proper stdio inheritance.The
spawncall usesstdio: "inherit"which correctly forwards input/output streams, andcwd: process.cwd()maintains the current working directory context.
97-106: Error handling for missing envio command is comprehensive.The error handling properly detects the
ENOENTcode for missing commands and provides helpful user feedback.
108-113: Exit code propagation maintains CLI contract.The script correctly propagates non-zero exit codes from the spawned
envio startprocess, maintaining proper CLI behavior.
| startProcessing, | ||
| } from "./Integration_ts_helpers.gen"; | ||
| // import { setLogLevel } from "generated/src/Logging.bs.js"; | ||
| // import { setLogLevel } from "generated/src/Logging.res.js"; |
There was a problem hiding this comment.
Fix commented import path
The commented import for setLogLevel uses generated/src/... without a leading .., which will fail when uncommented. Update it to:
// import { setLogLevel } from "../generated/src/Logging.res.js";
🤖 Prompt for AI Agents
In scenarios/test_codegen/test/integration-raw-events-test.ts at line 23, the
commented import statement for setLogLevel uses an incorrect relative path
without the leading "..". To fix this, update the import path to include the
leading ".." so it reads: import { setLogLevel } from
"../generated/src/Logging.res.js"; ensuring the path correctly points to the
generated directory.
| "codegen": "cargo run --manifest-path ../../codegenerator/cli/Cargo.toml -- codegen", | ||
| "dev": "pnpm build && envio dev", | ||
| "start": "pnpm build && node generated/src/Index.bs.js" | ||
| "start": "pnpm build && node generated/src/Index.res.js" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify no stale .bs.js references remain
Ensure other package.json scripts and documentation have been updated. Run:
🏁 Script executed:
#!/bin/bash
# Search for any remaining .bs.js entries
rg "generated/src/.*\.bs\.js" -A1Length of output: 481
Update stale .bs.js references in code generator templates
The search revealed that your change in scenarios/erc20_multichain_factory/package.json updated the start script to use .res.js, but there are still lingering .bs.js references in your code generator templates:
• codegenerator/cli/templates/static/codegen/src/Index.bs.js
– Contains comments and replace calls targeting generated/src/Index.bs.js
Please update these template files to reference Index.res.js (or remove the old .bs.js logic) so new projects won’t include stale .bs.js artifacts. You may also want to re-run:
rg "\.bs\.js"to confirm no other occurrences remain.
🤖 Prompt for AI Agents
In scenarios/erc20_multichain_factory/package.json at line 11, you updated the
start script to use generated/src/Index.res.js instead of .bs.js, but there are
still stale .bs.js references in the
codegenerator/cli/templates/static/codegen/src/Index.bs.js file. To fix this,
update the template file to replace all occurrences of Index.bs.js with
Index.res.js or remove any logic related to .bs.js files. After making these
changes, run a search for ".bs.js" again to ensure no other stale references
remain.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| /* tslint:disable */ | ||
|
|
||
| const EthersJS = require('./Ethers.bs.js'); | ||
| const EthersJS = require('./Ethers.res.js'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid manual edits in generated files
This file is auto-generated by genType. Instead of patching the require path here, update your genType template/config to emit .res.js imports so the change persists across regenerations.
🤖 Prompt for AI Agents
In codegenerator/cli/npm/envio/src/bindings/Ethers.gen.ts at line 6, the require
path is manually edited to './Ethers.res.js' in a generated file. Instead of
modifying this file directly, update the genType template or configuration to
output imports with the '.res.js' extension so that the change is preserved when
the file is regenerated.
| pub fn get_event_handler_directory(&self) -> String { | ||
| match self { | ||
| Language::ReScript => "./src/EventHandlers.bs.js".to_string(), | ||
| Language::ReScript => "./src/EventHandlers.res.js".to_string(), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify no remaining BuckleScript suffix references
Ensure all instances of the old .bs.js suffix have been removed across the codebase:
🏁 Script executed:
#!/bin/bash
rg '\.bs\.js' -nLength of output: 275
🏁 Script executed:
#!/bin/bash
rg '\.bs\.js' -n -A2 -B2 codegenerator/cli/templates/static/codegen/src/Index.bs.jsLength of output: 475
Fix remaining .bs.js suffix references in static codegen templates
The search for .bs.js found two hard-coded references in your codegen template that need updating:
• codegenerator/cli/templates/static/codegen/src/Index.bs.js
– Line 49: /ts-node\s+generated\/src\/Index\.bs\.js/g
– Line 55: /node\s+generated\/src\/Index\.bs\.js/g
Please update both patterns to use the new .res.js suffix, for example:
- newScript = newScript.replace(
- /ts-node\s+generated\/src\/Index\.bs\.js/g,
+ newScript = newScript.replace(
+ /ts-node\s+generated\/src\/Index\.res\.js/g,
- newScript = newScript.replace(
- /node\s+generated\/src\/Index\.bs\.js/g,
+ newScript = newScript.replace(
+ /node\s+generated\/src\/Index\.res\.js/g,Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In codegenerator/cli/src/cli_args/init_config.rs at line 343, update the
hard-coded file suffix from ".bs.js" to ".res.js" to remove any remaining
BuckleScript suffix references. Additionally, in the
codegenerator/cli/templates/static/codegen/src/Index.bs.js file, locate lines 49
and 55 where the regex patterns reference "Index.bs.js" and change these to
"Index.res.js" to ensure consistency with the new suffix convention.
| newScript = newScript.replace( | ||
| /ts-node\s+generated\/src\/Index\.bs\.js/g, | ||
| "envio start" | ||
| ); | ||
|
|
||
| // Replace node generated/src/Index.bs.js with envio start | ||
| newScript = newScript.replace( | ||
| /node\s+generated\/src\/Index\.bs\.js/g, | ||
| "envio start" | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Regex patterns should be more precise to avoid unintended replacements.
The current regex patterns could potentially match unintended strings. Consider making them more specific to avoid false positives.
- newScript = newScript.replace(
- /ts-node\s+generated\/src\/Index\.bs\.js/g,
- "envio start"
- );
+ newScript = newScript.replace(
+ /\bts-node\s+generated\/src\/Index\.bs\.js\b/g,
+ "envio start"
+ );
- newScript = newScript.replace(
- /node\s+generated\/src\/Index\.bs\.js/g,
- "envio start"
- );
+ newScript = newScript.replace(
+ /\bnode\s+generated\/src\/Index\.bs\.js\b/g,
+ "envio start"
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| newScript = newScript.replace( | |
| /ts-node\s+generated\/src\/Index\.bs\.js/g, | |
| "envio start" | |
| ); | |
| // Replace node generated/src/Index.bs.js with envio start | |
| newScript = newScript.replace( | |
| /node\s+generated\/src\/Index\.bs\.js/g, | |
| "envio start" | |
| ); | |
| newScript = newScript.replace( | |
| /\bts-node\s+generated\/src\/Index\.bs\.js\b/g, | |
| "envio start" | |
| ); | |
| // Replace node generated/src/Index.bs.js with envio start | |
| newScript = newScript.replace( | |
| /\bnode\s+generated\/src\/Index\.bs\.js\b/g, | |
| "envio start" | |
| ); |
🤖 Prompt for AI Agents
In codegenerator/cli/templates/static/codegen/src/Index.bs.js between lines 50
and 59, the regex patterns used to replace command strings are too broad and may
match unintended text. Refine the regex patterns by anchoring them to match the
exact command format, such as ensuring they match the entire command string or
include start/end boundaries, to prevent accidental replacements of similar but
unrelated strings.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
codegenerator/integration_tests/tests/runSingle.sh (1)
53-53: Optional: Capture the indexer PID for precise cleanup
Currently, cleanup relies on port-based detection; capturing the background process PID—for example:TUI_OFF="true" pnpm dev & PID=$! echo "Indexer PID: $PID"—would allow you to target the specific process and avoid potential collisions.
codegenerator/integration_tests/tests/testIndexerExits.sh (1)
47-47: Nit: Quote environment variable assignment
For consistency and stricter POSIX compliance, consider quoting the value:-export TUI_OFF=true +export TUI_OFF="true"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
codegenerator/cli/templates/static/blank_template/rescript/rescript.json(1 hunks)codegenerator/cli/templates/static/erc20_template/rescript/rescript.json(1 hunks)codegenerator/cli/templates/static/greeter_template/rescript/rescript.json(1 hunks)codegenerator/cli/templates/static/greeteronfuel_template/rescript/rescript.json(1 hunks)codegenerator/integration_tests/tests/runSingle.sh(1 hunks)codegenerator/integration_tests/tests/testIndexerExits.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- codegenerator/cli/templates/static/greeteronfuel_template/rescript/rescript.json
- codegenerator/cli/templates/static/blank_template/rescript/rescript.json
- codegenerator/cli/templates/static/greeter_template/rescript/rescript.json
- codegenerator/cli/templates/static/erc20_template/rescript/rescript.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
🔇 Additional comments (2)
codegenerator/integration_tests/tests/runSingle.sh (1)
53-53: Direct invocation of pnpm dev is aligned with PR objectives
By removing theenviowrapper and runningpnpm devwithTUI_OFF="true", the test now accurately reflects the intended startup command and avoids the previous dependency-hoisting issue.codegenerator/integration_tests/tests/testIndexerExits.sh (1)
48-48: Replace envio wrapper with direct pnpm start as intended
Executingpnpm startunderTUI_OFFmatches the new integration-test convention and ensures the indexer runs correctly without the envio CLI intermediary.
| "suffix": ".bs.js", | ||
| "bs-dependencies": ["generated", "envio"] | ||
| "suffix": ".res.js", | ||
| "bs-dependencies": ["generated", "envio", "rescript-schema"] |
There was a problem hiding this comment.
Need this for correct genType import path. Interesting that it's not required to add it to the indexer dependency.
|
|
||
| echo "Starting indexer" | ||
| TUI_OFF="true" $envio_cmd dev & | ||
| TUI_OFF="true" pnpm dev & |
There was a problem hiding this comment.
Otherwise it won't run rescript compiler for ReScript lang.
There was a problem hiding this comment.
Cool, just wondering if this removes coverage of the cli. I'm not too worried though its been quite stable
There was a problem hiding this comment.
I think it should be fine. We don't change the command anyways. I'd even say that it increases the coverage.
DZakh
changed the title
| # ReScript artifacts | ||
| *.bs.js | ||
| *.res.js | ||
| *.res.mjs |
There was a problem hiding this comment.
We might need to migrate to it in v3 when we start using ESM. I decided to add it beforehand.
| // Main execution | ||
| console.log("📋 Migration steps:"); | ||
| console.log(" 1. Update package.json scripts"); | ||
| console.log(" 2. Run envio start"); | ||
| console.log(""); |
There was a problem hiding this comment.
I'm not sure about automatically running this, surely it should check and then prompt a user?
There was a problem hiding this comment.
I think it should be fine.
| "log_index", | ||
| "block_timestamp", | ||
| "is_pre_registering_dynamic_contracts", | ||
| "is_pre_registering_dynamic_contracts" |
There was a problem hiding this comment.
I'm sure we want to eventually refactor this file entirely but we should probably have a prettier config file so we don't see this stuff changing between commits
| @@ -0,0 +1,135 @@ | |||
| #!/usr/bin/env node | |||
There was a problem hiding this comment.
@DZakh maybe you can also include a comment at the top of this file to explain why it exists.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
codegenerator/cli/templates/static/codegen/src/Index.bs.js (2)
65-74: Regex patterns are still too broad and could cause unintended replacements.This is the same issue flagged in previous reviews. The current patterns could match unintended strings within larger commands or file paths.
Apply the previously suggested fix to make the regex patterns more precise:
- newScript = newScript.replace( - /ts-node\s+generated\/src\/Index\.bs\.js/g, - "envio start" - ); + newScript = newScript.replace( + /\bts-node\s+generated\/src\/Index\.bs\.js\b/g, + "envio start" + ); - newScript = newScript.replace( - /node\s+generated\/src\/Index\.bs\.js/g, - "envio start" - ); + newScript = newScript.replace( + /\bnode\s+generated\/src\/Index\.bs\.js\b/g, + "envio start" + );
1-1:⚠️ Potential issueFilename contradicts the migration purpose.
This migration script is designed to move away from
.bs.jsfiles to.res.jsfiles, yet the script itself uses the.bs.jsextension. This creates confusion and contradicts the migration objective.Consider renaming this file to use a more appropriate extension:
-Index.bs.js +Index.jsOr if you need to maintain the ReScript connection for some reason:
-Index.bs.js +Index.res.js
🧹 Nitpick comments (2)
codegenerator/cli/templates/static/codegen/src/Index.bs.js (2)
112-118: Timeout behavior might confuse users.The 30-second timeout that automatically skips migration without user awareness could be confusing, especially since users might not notice the timeout message.
Consider providing a clearer indication when the timeout is approaching:
+ // Warn user about timeout after 20 seconds + const warningTimeout = setTimeout(() => { + console.log("\n⏰ Migration prompt will timeout in 10 seconds..."); + }, 20000); + const timeout = setTimeout(() => { + clearTimeout(warningTimeout); rl.close(); console.log( "\n⏱️ No response received in 30 seconds, skipping package.json migration." ); resolve(false); }, 30000); rl.question( "🤔 Would you like to automatically update your package.json scripts? (y/N): ", (answer) => { + clearTimeout(warningTimeout); clearTimeout(timeout);
144-153: Error handling could be more informative.While the ENOENT error is handled well, other potential errors could benefit from more specific guidance.
Consider adding more specific error handling:
envioProcess.on("error", (error) => { if (error.code === "ENOENT") { console.error( '❌ Error: "envio" command not found. Please make sure Envio CLI is installed.' ); + console.error(" Try running: npm install -g @envio-dev/envio"); + } else if (error.code === "EACCES") { + console.error("❌ Error: Permission denied. Try running with appropriate permissions."); } else { console.error("❌ Error running envio start:", error.message); } process.exit(1); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
codegenerator/cli/templates/static/codegen/src/Index.bs.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
🔇 Additional comments (1)
codegenerator/cli/templates/static/codegen/src/Index.bs.js (1)
3-17: Excellent documentation explaining the migration purpose.The comprehensive comment block clearly explains the context, breaking changes, and migration steps. This addresses previous feedback about including explanatory comments.
| const shouldMigrate = | ||
| answer.toLowerCase() === "y" || | ||
| answer.toLowerCase() === "yes" || | ||
| answer.trim() === ""; | ||
| resolve(shouldMigrate); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Empty input defaulting to "yes" may be confusing to users.
The current logic treats an empty string (just pressing Enter) as agreeing to the migration. This might be unexpected behavior for users who expect explicit confirmation.
Consider making the default behavior more explicit:
- const shouldMigrate =
- answer.toLowerCase() === "y" ||
- answer.toLowerCase() === "yes" ||
- answer.trim() === "";
+ const shouldMigrate =
+ answer.toLowerCase() === "y" ||
+ answer.toLowerCase() === "yes";And update the prompt to indicate the default:
- "🤔 Would you like to automatically update your package.json scripts? (y/N): ",
+ "🤔 Would you like to automatically update your package.json scripts? (y/n): ",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const shouldMigrate = | |
| answer.toLowerCase() === "y" || | |
| answer.toLowerCase() === "yes" || | |
| answer.trim() === ""; | |
| resolve(shouldMigrate); | |
| rl.question( | |
| // Updated prompt to show no “yes” default. | |
| "🤔 Would you like to automatically update your package.json scripts? (y/n): ", | |
| (answer: string) => { | |
| // Only “y” or “yes” now count as confirmation; empty no longer defaults to yes. | |
| const shouldMigrate = | |
| answer.toLowerCase() === "y" || | |
| answer.toLowerCase() === "yes"; | |
| resolve(shouldMigrate); | |
| } | |
| ); |
🤖 Prompt for AI Agents
In codegenerator/cli/templates/static/codegen/src/Index.bs.js around lines 125
to 129, the code treats empty input as a "yes" response, which can confuse
users. Modify the logic to not default empty input to "yes" by removing the
condition that treats an empty string as confirmation. Additionally, update the
user prompt to clearly indicate what the default choice is or require explicit
confirmation to proceed.
Fixes: https://github.com/enviodev/hyperindex/actions/runs/15489582249/job/43611811130
Summary by CodeRabbit
New Features
Bug Fixes
.res.jsextension instead of.bs.js..res.jsfiles for improved compatibility.Chores
.gitignoreand configuration files to align with new ReScript output conventions.Documentation
Refactor