fix: fix several issues with JS bindings #131
Conversation
These are just the header row but we parse the rows and emit them as structured logs anyway.
When I tried to use the existing module within a Webpack project I found that the generator was continuing with promises and trying to initialize WebAssembly using them, which obviously failed. This function seems to work a lot better for some reason, although I haven't digged into why, exactly.
WalkthroughThe pull request introduces several changes across multiple files. The GitHub Actions workflow configuration is updated to include Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
components/js/prophet-wasmstan/run.js (2)
1-9: Appreciate the detailed comment block and consider creating an issue forjco transpile.The comment block provides valuable context about the function's origin and implementation. It's great to see the attribution to the Stack Overflow source and the explanation of how this function is integrated into the build process.
However, the note about
jco transpilenot working as expected is concerning.Would you like me to create a GitHub issue to track the investigation of why the
jco transpilegenerated function didn't work as expected? This could help improve the tooling and potentially remove the need for this custom implementation in the future.
10-16: Approve the implementation with a minor suggestion for readability.The
runfunction effectively converts a generator function into a promise, handling both synchronous and asynchronous yields. The implementation is concise and follows good practices for working with generators and promises.For improved readability, consider adding a comment explaining the purpose of the
stepfunction and how it processes the generator's yields.Here's a suggested addition to improve the documentation:
function run(g) { + // `step` function: Recursively processes each yield from the generator + // and returns a promise that resolves to the generator's final value return Promise.resolve(function step(v) { const res = g.next(v); if (res.done) return res.value; return res.value.then(step); }()); }crates/augurs-prophet/src/positive_float.rs (1)
51-61: LGTM: Custom deserialization logic is well-implemented, with a minor suggestion.The custom
serde::Deserializeimplementation forPositiveFloatis well-designed:
- It correctly deserializes the input as an
f64.- It uses the
try_newmethod to create aPositiveFloat, ensuring proper validation.- It maps any errors to a custom serde error, preventing invalid states.
This implementation maintains the type's invariants during deserialization, which is crucial for
PositiveFloat.Consider preserving the original error message from
TryFromFloatErrorin the deserialization error for improved debugging. You could modify the error mapping as follows:Self::try_new(f).map_err(|e| serde::de::Error::custom(format!("Failed to create PositiveFloat: {}", e)))This change would provide more detailed error information during deserialization, potentially easing debugging in the future.
components/justfile (1)
Line range hint
1-89: Overall assessment of changes to the build processThe modifications to the
justfileappear to streamline the build and publish process by consolidating the transpilation and minification steps. While these changes can lead to a more efficient workflow, they also introduce some potential risks that should be addressed:
- The new post-processing step in the
transpiletarget adds complexity and a dependency onripgrep.- The removal of the
transpile-mintarget might have unintended consequences if it performed any crucial operations not captured in the new process.To ensure the integrity and reliability of the build process, consider the following recommendations:
- Document the new build process thoroughly, explaining the rationale behind the changes and any new dependencies (like
ripgrep).- Create a comprehensive test suite that verifies the output of the new build process, ensuring it matches expectations in terms of functionality, file size, and content.
- Consider containerizing the build environment to ensure consistency across different systems and to manage dependencies like
ripgrepmore effectively.- Implement a staged rollout of these changes, perhaps maintaining the old build process in parallel for a short period to allow for easy rollback if issues are discovered.
- Update any CI/CD pipelines to accommodate the new build process and include checks for the presence of required tools like
ripgrep.By addressing these points, you can mitigate the risks associated with these changes while benefiting from the streamlined build process.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- .github/workflows/wasmstan.yml (1 hunks)
- components/js/prophet-wasmstan/run.js (1 hunks)
- components/justfile (1 hunks)
- crates/augurs-js/src/prophet.rs (4 hunks)
- crates/augurs-prophet/src/positive_float.rs (2 hunks)
🧰 Additional context used
🪛 Biome
components/js/prophet-wasmstan/run.js
[error] 17-17: Illegal return statement outside of a function
(parse)
🔇 Additional comments (7)
.github/workflows/wasmstan.yml (1)
23-23: LGTM! Verify ripgrep usage in the project.The addition of
ripgrepto the tool installation list is a good improvement. It's a powerful search tool that can enhance the workflow's capabilities.To ensure this addition is utilized effectively, let's verify its usage in the project:
This will help confirm that
ripgrepis being utilized in the project, justifying its inclusion in the workflow.components/js/prophet-wasmstan/run.js (1)
17-17:⚠️ Potential issueClarify the context for the standalone return statement.
There's a return statement
return run(gen);outside of any function context, which is syntactically incorrect in JavaScript. This suggests that this file might be intended to be part of a larger function or module.Could you please clarify the intended context for this code? If this file is meant to be a module, consider wrapping the entire content in a function or removing this line if it's not needed.
To help understand the usage of this file, let's check for its imports across the project:
🧰 Tools
🪛 Biome
[error] 17-17: Illegal return statement outside of a function
(parse)
crates/augurs-prophet/src/positive_float.rs (1)
5-5: LGTM: Removal of automatic deserialization is appropriate.The removal of the
serde::Deserializederive macro is a good decision. This change allows for the implementation of custom deserialization logic, which is necessary for types likePositiveFloatthat require validation during deserialization. Theserde::Serializederive macro is retained, ensuring that serialization behavior remains unchanged.components/justfile (2)
77-84: Consider the implications of post-processing the transpiled JavaScriptThe new lines in the
transpiletarget introduce a post-processing step usingripgrep(rg) to modify the transpiled JavaScript file. While this approach can be effective for injecting custom code or fixing issues, it has several implications:
- It may alter the behavior of the transpiled code in ways that are not immediately obvious.
- It introduces a dependency on
ripgrep, which might not be available in all build environments.- The use of complex regex patterns in build scripts can be error-prone and challenging to maintain.
Consider the following suggestions:
- Document the purpose of this post-processing step to ensure future maintainers understand its importance.
- Ensure that
ripgrepis installed as part of the build environment setup.- If possible, address the root cause of why this post-processing is necessary. Could the issue be resolved in the original source code or the transpilation process?
- If this post-processing is absolutely necessary, consider creating a separate script (e.g., in Python or Node.js) to handle this transformation. This would make the logic easier to test and maintain.
To ensure
ripgrepis available in the build environment, you can run:✅ Verification successful
Dependency Check Passed: ripgrep is installed
ripgrep version 13.0.0 is installed in the build environment as required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if ripgrep is installed if ! command -v rg &> /dev/null then echo "ripgrep is not installed. Please install it to proceed with the build." exit 1 else echo "ripgrep is installed. Version: $(rg --version | head -n 1)" fiLength of output: 145
89-89: Verify the impact of removing thetranspile-minstepThe
publishtarget now depends ontranspileinstead oftranspile-min. This change, along with the removal of thetranspile-mintarget (as mentioned in the AI summary), simplifies the build process. However, it's important to ensure that all necessary steps are still being performed.Please confirm the following:
- Does the new
transpiletarget include all necessary optimization or minification steps that were previously intranspile-min?- Are there any differences in the output files (size, content, structure) compared to the previous build process?
- Have you tested the published package to ensure it functions correctly with these changes?
To help verify the impact, you can run the following script to compare the output of the old and new build processes:
This script will help identify any significant changes in the build output and ensure that all references to the removed
transpile-mintarget have been updated.crates/augurs-js/src/prophet.rs (2)
477-478: Great improvement in type safety!The change from
f64toPositiveFloatfor thesigma_obsfield is an excellent improvement. It enforces thatsigma_obsis always positive, which is likely a domain-specific requirement. The#[tsify(type = "number")]attribute ensures that the TypeScript type remains a number, maintaining compatibility with the JavaScript/TypeScript interface while improving type safety in Rust.
Line range hint
420-453: LGTM! Clarification needed onemitted_headerremoval.The refactoring of the
emitmethod improves code clarity and efficiency. The direct iteration over log lines and inline filtering of "Iter" messages are good improvements. However, could you please clarify the intention behind removing theemitted_headerfield? Was this intentional, and does it affect the logging behavior in any way?To verify the impact of removing the
emitted_headerfield, please run the following script:✅ Verification successful
Removal of
emitted_headerConfirmedThe
emitted_headerfield has been successfully removed from the codebase, and no further usages were found. This refactoring enhances code clarity without affecting logging behavior.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any other usages of `emitted_header` in the codebase rg --type rust 'emitted_header'Length of output: 31
Script:
#!/bin/bash # Description: Perform a comprehensive search for any occurrences of `emitted_header`, including case variations and within comments. rg --type rust --ignore-case 'emitted_header'Length of output: 45
See commits for details.
Summary by CodeRabbit
PositiveFloatstruct for custom deserialization logic.transpile-mintarget and integrated its functionality into thetranspiletarget.publishtarget to depend on the modifiedtranspiletarget.