WIP: chore: move farm plugins #2267
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR introduces a new YAML plugin for FarmFE that detects and transforms YAML files into JavaScript modules. It includes a Rust implementation with platform-specific NPM binaries, Node.js integration scripts for binary resolution, and comprehensive documentation across multiple supported platforms (macOS, Linux, Windows). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes Areas requiring extra attention:
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
Maidang1
force-pushed
the
chore/plugins
branch
from
03136fd to
16e1841
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (4)
rust-plugins/yaml/npm/darwin-arm64/package.json (1)
1-18: Version alignment required (duplicate of main package issue).This platform package is versioned
0.0.2, but should be aligned with the main package version2.0.0-beta.0. See main package.json review comment.rust-plugins/yaml/npm/win32-arm64-msvc/package.json (1)
1-18: Version alignment required (duplicate of main package issue).This platform package is versioned
0.0.2and should be aligned with the main package version. See main package.json review comment.rust-plugins/yaml/npm/win32-x64-msvc/package.json (1)
1-18: Version alignment required (duplicate of main package issue).This platform package is versioned
0.0.2and should be aligned with the main package version. See main package.json review comment.rust-plugins/yaml/npm/linux-x64-musl/package.json (1)
1-18: Version alignment required (duplicate of main package issue).This platform package is versioned
0.0.2and should be aligned with the main package version. See main package.json review comment.
🧹 Nitpick comments (9)
rust-plugins/yaml/scripts/watch.sh (1)
1-3: Add error handling to fail fast and clarify intent.The script silently runs
npm run buildand always exits successfully, potentially masking build failures. Additionally, the filename "watch.sh" is misleading for a one-time build runner—typical watch scripts auto-rebuild on file changes.If this is intended as a one-time build helper, consider either renaming it or adding error handling and a comment:
#!/bin/bash + +set -e # Exit on error npm run buildAlternatively, if it's meant to support file watching during development, implement auto-rebuild logic:
#!/bin/bash + +set -e + +# Watch and rebuild on changes +npm run build -- --watchrust-plugins/yaml/Cargo.toml (1)
1-17: Manifest is fine; consider aligning metadata and dependency versioningThe Cargo manifest is valid and sufficient for an internal crate. If this crate is intended to be published or reused more broadly, consider:
- Adding
description,license, and possiblyrepositoryunder[package].- If the workspace already standardizes versions for
farmfe_core,farmfe_macro_plugin, andfarmfe_toolkit_plugin_types, mirroring that pattern (e.g., via workspace inheritance) instead ofversion = "*", for consistency and easier dependency management.These are polish items, not blockers.
rust-plugins/yaml/options.d.ts (1)
1-3: EmptyIPluginOptionsloses useful typing; mirror actual YAML plugin optionsRight now
IPluginOptionsis empty, so consumers get no type help and it can easily diverge from the real Rust-sideFarmPluginYamlOptions(include/exclude filters etc.). It would be better to expose the actual supported options here, e.g. along the lines of:export interface IPluginOptions { include?: string | RegExp | Array<string | RegExp>; exclude?: string | RegExp | Array<string | RegExp>; // ...any other options supported by FarmPluginYamlOptions }This keeps TS declarations and the Rust implementation in sync and improves DX.
rust-plugins/yaml/npm/linux-x64-gnu/package.json (1)
1-21: Linux x64 GNU binary package.json is consistent with common patternsThe manifest fields (name, os/cpu/libc constraints,
main: "index.farm",files) look correct for a platform-specific binary package. If you want extra polish, you could addrepositoryandhomepagefor easier discovery, but it’s not required.rust-plugins/yaml/scripts/func.js (1)
1-3: VerifybinPathexport shape and tuple contractThis wrapper assumes that
./index.jshas a default exportbinPathand that the consumer expects[binPath, options]wherebinPathis that default export as‑is.Two things worth double‑checking:
Export form of
./index.js
Ensureindex.jsactually exposesbinPathas its default export with the intended shape (string vs function). Ifindex.jsis CJS and doesmodule.exports = { binPath }, the default import here will be an object, not the function/string itself, and the tuple will be wrong.Expected tuple element type
Confirm the plugin loader expects the first element to be exactly whatbinPathis (and not, for example, the result of calling abinPath(options)function). If the contract is[binPath(options), options], you’d want:import binPath from "./index.js"; export default (options) => [binPath(options), options];Given this is just a thin adapter, a quick check against the other Rust plugin wrappers in the repo to match their pattern would avoid subtle interop bugs.
rust-plugins/yaml/npm/linux-arm64-gnu/package.json (1)
1-21: Linux ARM64 GNU binary package.json mirrors the x64 variant correctlyThe platform selectors (
os: ["linux"],cpu: ["arm64"],libc: ["glibc"]) and entrypoint configuration match the intended ARM64 GNU binary packaging; this looks good. As with the x64 package, addingrepository/homepageis optional polish.rust-plugins/yaml/scripts/index.js (1)
27-122: Consider extracting a helper to reduce repetition.The pattern
existsSync(local) ? local : require.resolve(pkg)is repeated for each platform/arch. A helper function could simplify this.function resolveBinary(localPath, packageName) { if (existsSync(join(currentDir, localPath))) { return join(currentDir, localPath); } return require.resolve(packageName); } // Usage example: binPath = resolveBinary('../npm/win32-x64-msvc/index.farm', '@farmfe/plugin-yaml-win32-x64-msvc');This is a low-priority suggestion; the current explicit approach is also readable.
rust-plugins/yaml/src/lib.rs (2)
18-20: Replacelazy_static!with a simpleconst.Using
lazy_static!for a constant string adds unnecessary runtime overhead. Since this value never changes, it should be a compile-time constant.Apply this diff:
-lazy_static! { - static ref YAML_MODULE_TYPE: String = String::from("yaml"); -} +const YAML_MODULE_TYPE: &str = "yaml";Then update line 121 and 132 to use
YAML_MODULE_TYPEdirectly (it will already be a&strthat can be used withto_string()).
22-24: Accept&strinstead of&Stringfor better ergonomics.The function parameter can accept
&strwhich is more idiomatic and flexible in Rust.Apply this diff:
-fn is_yaml_file(file_name: &String) -> bool { +fn is_yaml_file(file_name: &str) -> bool { file_name.ends_with(".yaml") || file_name.ends_with(".yml") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yamlrust-plugins/yaml/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
rust-plugins/yaml/.gitignore(1 hunks)rust-plugins/yaml/CHANGELOG.md(1 hunks)rust-plugins/yaml/Cargo.toml(1 hunks)rust-plugins/yaml/npm/darwin-arm64/README.md(1 hunks)rust-plugins/yaml/npm/darwin-arm64/package.json(1 hunks)rust-plugins/yaml/npm/darwin-x64/README.md(1 hunks)rust-plugins/yaml/npm/darwin-x64/package.json(1 hunks)rust-plugins/yaml/npm/linux-arm64-gnu/README.md(1 hunks)rust-plugins/yaml/npm/linux-arm64-gnu/package.json(1 hunks)rust-plugins/yaml/npm/linux-arm64-musl/README.md(1 hunks)rust-plugins/yaml/npm/linux-arm64-musl/package.json(1 hunks)rust-plugins/yaml/npm/linux-x64-gnu/README.md(1 hunks)rust-plugins/yaml/npm/linux-x64-gnu/package.json(1 hunks)rust-plugins/yaml/npm/linux-x64-musl/README.md(1 hunks)rust-plugins/yaml/npm/linux-x64-musl/package.json(1 hunks)rust-plugins/yaml/npm/win32-arm64-msvc/README.md(1 hunks)rust-plugins/yaml/npm/win32-arm64-msvc/package.json(1 hunks)rust-plugins/yaml/npm/win32-ia32-msvc/README.md(1 hunks)rust-plugins/yaml/npm/win32-ia32-msvc/package.json(1 hunks)rust-plugins/yaml/npm/win32-x64-msvc/README.md(1 hunks)rust-plugins/yaml/npm/win32-x64-msvc/package.json(1 hunks)rust-plugins/yaml/options.d.ts(1 hunks)rust-plugins/yaml/package.json(1 hunks)rust-plugins/yaml/readme.md(1 hunks)rust-plugins/yaml/rustfmt.toml(1 hunks)rust-plugins/yaml/scripts/func.js(1 hunks)rust-plugins/yaml/scripts/index.d.ts(1 hunks)rust-plugins/yaml/scripts/index.js(1 hunks)rust-plugins/yaml/scripts/watch.sh(1 hunks)rust-plugins/yaml/src/lib.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-01T08:01:11.021Z
Learnt from: fu050409
Repo: farm-fe/farm PR: 2234
File: biome.json:41-41
Timestamp: 2025-09-01T08:01:11.021Z
Learning: When vcs.useIgnoreFile is true in Biome configuration, explicit excludes for node_modules and dist directories in formatter includes are redundant since Biome automatically respects .gitignore.
Applied to files:
rust-plugins/yaml/.gitignore
🧬 Code graph analysis (3)
rust-plugins/yaml/scripts/index.d.ts (2)
rust-plugins/yaml/scripts/func.js (1)
options(3-3)rust-plugins/yaml/options.d.ts (1)
IPluginOptions(1-3)
rust-plugins/yaml/scripts/func.js (1)
rust-plugins/yaml/scripts/index.js (1)
binPath(9-9)
rust-plugins/yaml/src/lib.rs (1)
crates/macro_plugin/src/lib.rs (1)
farm_plugin(6-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: Test - windows-latest
- GitHub Check: Test - ubuntu-latest
- GitHub Check: Check
- GitHub Check: Test - macos-latest
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm64-gnu
- GitHub Check: call-rust-build / Build and Upload Artifacts - android-arm64
- GitHub Check: call-rust-build / Build and Upload Artifacts - darwin-x64
- GitHub Check: call-rust-build / Build and Upload Artifacts - android-arm-eabi
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm-gnueabihf
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm64-musl
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-arm64-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-x64-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-x64-musl
- GitHub Check: call-rust-build / Build and Upload Artifacts - darwin-arm64
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-ia32-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-x64-gnu
- GitHub Check: call-rust-build / Build and Upload Artifacts - x86_64-unknown-freebsd
- GitHub Check: Rust Code Lint
🔇 Additional comments (16)
rust-plugins/yaml/npm/linux-x64-musl/README.md (1)
1-3: LGTM.The README accurately documents the platform-specific binary. Rust triple x86_64-unknown-linux-musl is correct for 64-bit x86 Linux with musl libc.
rust-plugins/yaml/npm/linux-arm64-gnu/README.md (1)
1-3: LGTM.The README accurately documents the platform-specific binary. Rust triple aarch64-unknown-linux-gnu is correct for ARM64 Linux with glibc.
rust-plugins/yaml/npm/win32-arm64-msvc/README.md (1)
1-3: LGTM.The README accurately documents the platform-specific binary. Rust triple aarch64-pc-windows-msvc is correct for ARM64 Windows with MSVC.
rust-plugins/yaml/npm/win32-x64-msvc/README.md (1)
1-3: LGTM.The README accurately documents the platform-specific binary. Rust triple x86_64-pc-windows-msvc is correct for 64-bit x86 Windows with MSVC.
rust-plugins/yaml/npm/linux-x64-gnu/README.md (1)
1-3: LGTM.The README accurately documents the platform-specific binary. Rust triple x86_64-unknown-linux-gnu is correct for 64-bit x86 Linux with glibc.
rust-plugins/yaml/rustfmt.toml (1)
1-2: LGTM.Rustfmt configuration is standard and correctly aligns with Rust 2021 edition. Indentation setting (2 spaces) is appropriate and consistent.
rust-plugins/yaml/npm/win32-ia32-msvc/package.json (2)
15-17: Verify Node.js engine requirement is intentional.The package specifies Node
>= 10, which reached end-of-life in April 2021. While broad compatibility is valuable, this may be outdated depending on your minimum supported Node version. Please confirm this is intentional or consider updating to a more recent minimum (e.g.,>= 14or>= 16).
1-18: Manifest structure looks good.The platform-specific npm package manifest is well-formed, with appropriate OS/CPU constraints and file declarations. The version "0.0.2" is consistent with the pre-release/migration context of this PR.
rust-plugins/yaml/npm/darwin-x64/README.md (1)
1-3: Documentation is clear and follows platform-package conventions.The README accurately describes the platform binary. No concerns.
rust-plugins/yaml/npm/win32-ia32-msvc/README.md (1)
1-3: Documentation is clear and follows platform-package conventions.The README accurately describes the platform binary. No concerns.
rust-plugins/yaml/.gitignore (1)
1-198: Gitignore is well-structured with appropriate multi-platform coverage.The file includes comprehensive templates for Node, macOS, Windows, and Cargo with proper exception patterns for Yarn workspaces and plugin artifacts (*.farm, *.node). Follows best practices.
rust-plugins/yaml/CHANGELOG.md (1)
1-61: Changelog content and structure look goodVersion ordering and sectioning are clear and conventional; nothing to change here.
rust-plugins/yaml/npm/linux-arm64-musl/README.md (1)
1-3: Platform‑specific README is clear and accurateName and target (
aarch64-unknown-linux-musl) match the package’s purpose; no changes needed.rust-plugins/yaml/npm/darwin-arm64/README.md (1)
1-3: Darwin ARM64 README looks goodThe package name and
aarch64-apple-darwindescription are consistent and sufficient for this binary wrapper.rust-plugins/yaml/npm/darwin-x64/package.json (1)
1-18: LGTM!The package manifest is correctly configured for macOS x64 with appropriate
os,cpu, and engine constraints.rust-plugins/yaml/scripts/index.js (1)
13-25: LGTM!The
isMusl()detection logic correctly handles both modern Node.js (viaprocess.report) and older versions (via/usr/bin/lddinspection) with a sensible fallback to musl on read errors (common in Alpine containers).
| "libc": [ | ||
| "glibc" | ||
| ] |
There was a problem hiding this comment.
Incorrect libc value for musl package.
The package is named linux-arm64-musl but declares libc: ["glibc"]. This should be ["musl"] to ensure npm installs the correct binary on musl-based systems like Alpine Linux.
"libc": [
- "glibc"
+ "musl"
]📝 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.
| "libc": [ | |
| "glibc" | |
| ] | |
| "libc": [ | |
| "musl" | |
| ] |
🤖 Prompt for AI Agents
In rust-plugins/yaml/npm/linux-arm64-musl/package.json around lines 18 to 20 the
package declares `"libc": ["glibc"]` which is incorrect for a musl-targeted
build; change the libc array value to `["musl"]` so the package metadata
correctly identifies musl-based systems and npm will install the proper binary.
| @@ -0,0 +1,58 @@ | |||
| { | |||
| "name": "@farmfe/plugin-yaml", | |||
| "version": "2.0.0-beta.0", | |||
There was a problem hiding this comment.
Resolve version mismatch between main and platform packages.
The main package is versioned 2.0.0-beta.0, but all platform-specific packages (darwin-arm64, darwin-x64, linux-x64-musl, win32-*, etc.) are versioned 0.0.2. This inconsistency will cause npm resolution and versioning misalignment during packaging and publishing.
Align all platform packages to version 2.0.0-beta.0 to match the main package version.
🤖 Prompt for AI Agents
In rust-plugins/yaml/package.json around line 3, ensure all platform-specific
package.json files that currently show version "0.0.2" (darwin-arm64,
darwin-x64, linux-x64-musl, win32-*, etc.) are updated to "2.0.0-beta.0" so they
match the main package version; open each platform package.json and replace the
version field value with "2.0.0-beta.0", save changes, and re-run your
packaging/build steps to confirm no version mismatch remains.
| "types": "scripts/index.d.ts", | ||
| "type": "module", | ||
| "license": "MIT", | ||
| "homepage": "https://github.com/farm-fe/plugins", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n rust-plugins/yaml/package.json | head -25Repository: farm-fe/farm
Length of output: 814
🏁 Script executed:
#!/bin/bash
echo "=== Searching for farm-fe/plugins references ==="
rg 'farm-fe/plugins' --type json -n
echo "=== Searching for farm-fe/farm references in package.json files ==="
rg 'farm-fe/farm' --type json -n
echo "=== Checking all package.json repository fields ==="
fd 'package.json$' -x grep -l 'repository' {}Repository: farm-fe/farm
Length of output: 7451
🏁 Script executed:
echo "=== Checking root package.json ==="
cat -n package.json | head -30Repository: farm-fe/farm
Length of output: 1634
Update repository metadata URLs from farm-fe/plugins to farm-fe/farm.
The homepage, bugs.url, and repository.url fields currently reference the old farm-fe/plugins location. Update these to use farm-fe/farm to be consistent with other packages in the monorepo:
- Line 9: Change
homepagetohttps://github.com/farm-fe/farm/tree/main/rust-plugins/yaml - Line 12: Change
bugs.urltohttps://github.com/farm-fe/farm/issues - Line 16: Change
repository.urltogit+https://github.com/farm-fe/farm.git
🤖 Prompt for AI Agents
In rust-plugins/yaml/package.json around lines 9, 12 and 16, update repository
metadata URLs that still reference farm-fe/plugins: change the homepage value on
line 9 to "https://github.com/farm-fe/farm/tree/main/rust-plugins/yaml", change
the bugs.url on line 12 to "https://github.com/farm-fe/farm/issues", and change
the repository.url on line 16 to "git+https://github.com/farm-fe/farm.git".
| "napi": { | ||
| "binaryName": "farm-plugin-yaml", | ||
| "triples": { | ||
| "additional": [ | ||
| "x86_64-unknown-linux-gnu", | ||
| "x86_64-pc-windows-msvc", | ||
| "x86_64-apple-darwin", | ||
| "aarch64-apple-darwin", | ||
| "aarch64-unknown-linux-gnu", | ||
| "aarch64-unknown-linux-musl", | ||
| "x86_64-unknown-linux-musl", | ||
| "i686-pc-windows-msvc", | ||
| "aarch64-pc-windows-msvc" | ||
| ] | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, explore the rust-plugins/yaml directory structure
find rust-plugins/yaml -type f -name "package.json" | head -20Repository: farm-fe/farm
Length of output: 535
🏁 Script executed:
# Check the main package.json version
echo "=== Main package.json version ==="
cat rust-plugins/yaml/package.json | grep -A 2 '"version"'
echo -e "\n=== Sample npm package versions ==="
# Check a few npm package.json files for version alignment
for pkg in darwin-arm64 linux-x64-gnu win32-x64-msvc; do
echo "--- $pkg ---"
cat "rust-plugins/yaml/npm/$pkg/package.json" | grep -A 2 '"version"' | head -3
doneRepository: farm-fe/farm
Length of output: 404
🏁 Script executed:
# Check all npm package versions
echo "=== All npm package versions ==="
for dir in rust-plugins/yaml/npm/*/; do
pkg_name=$(basename "$dir")
version=$(grep '"version"' "$dir/package.json" | head -1 | sed 's/.*"version": "\([^"]*\)".*/\1/')
echo "$pkg_name: $version"
doneRepository: farm-fe/farm
Length of output: 285
Fix version mismatch between main package and platform-specific npm packages.
All 9 NAPI platform triples have corresponding npm packages (darwin-arm64, darwin-x64, linux-arm64-gnu, linux-arm64-musl, linux-x64-gnu, linux-x64-musl, win32-arm64-msvc, win32-ia32-msvc, win32-x64-msvc). However, the main package.json is versioned at 2.0.0-beta.0 while all platform-specific npm packages are versioned at 0.0.2. Ensure version consistency across all package manifests.
🤖 Prompt for AI Agents
rust-plugins/yaml/package.json lines 26-41: the main package.json is at version
2.0.0-beta.0 while the platform-specific npm packages are at 0.0.2 causing a
mismatch; update the version fields in all platform-specific package manifests
(the NAPI platform packages: darwin-arm64, darwin-x64, linux-arm64-gnu,
linux-arm64-musl, linux-x64-gnu, linux-x64-musl, win32-arm64-msvc,
win32-ia32-msvc, win32-x64-msvc) to match the main package (2.0.0-beta.0), and
verify any build/publish scripts and package metadata reference the same version
so all published artifacts are consistent.
| *notice:* | ||
|
|
||
| include or exclude is Regex not glob For example `**/01.yaml` is not illegal。What is right is like `".*\\/01.yaml"` |
There was a problem hiding this comment.
Fix grammatical error and punctuation.
The phrase "is not illegal" should be "is illegal" or "is not valid". Also, the sentence ends with a Chinese period "。" instead of a standard period.
-include or exclude is Regex not glob For example `**/01.yaml` is not illegal。What is right is like `".*\\/01.yaml"`
+include or exclude is Regex, not glob. For example, `**/01.yaml` is not valid. Use something like `".*\\/01.yaml"` instead.📝 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.
| *notice:* | |
| include or exclude is Regex not glob For example `**/01.yaml` is not illegal。What is right is like `".*\\/01.yaml"` | |
| *notice:* | |
| include or exclude is Regex, not glob. For example, `**/01.yaml` is not valid. Use something like `".*\\/01.yaml"` instead. |
🤖 Prompt for AI Agents
In rust-plugins/yaml/readme.md around lines 34-36, fix the grammar and
punctuation in the sentence explaining pattern types: change "include or exclude
is Regex not glob For example `**/01.yaml` is not illegal。" to a clear, properly
punctuated sentence such as "Include or exclude are regular expressions, not
globs. For example, a regex like \\.*/01.yaml is valid while glob patterns like
**/01.yaml are not." Replace the Chinese period with a standard period,
capitalize appropriately, and use "regular expressions" or "regex" and "not
valid" (or "not allowed") instead of "is not illegal."
| fn should_process_path(&self, path: &str) -> bool { | ||
| if !self.include.is_empty() { | ||
| let inc_reg = match Regex::new(&self.include) { | ||
| Ok(reg) => reg, | ||
| Err(_) => return false, | ||
| }; | ||
| if inc_reg.find(path).is_none() { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| if !self.exclude.is_empty() { | ||
| let exc_reg = match Regex::new(&self.exclude) { | ||
| Ok(reg) => reg, | ||
| Err(_) => return true, | ||
| }; | ||
| if exc_reg.find(path).is_some() { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| true | ||
| } |
There was a problem hiding this comment.
Invalid regex patterns fail silently, hiding configuration errors.
Lines 67 and 77 silently ignore invalid regex patterns, which means users won't know their include/exclude configuration is broken. This can lead to unexpected behavior where files are processed or skipped without clear indication why.
Consider logging a warning or returning an error when regex compilation fails:
fn should_process_path(&self, path: &str) -> bool {
if !self.include.is_empty() {
let inc_reg = match Regex::new(&self.include) {
Ok(reg) => reg,
- Err(_) => return false,
+ Err(e) => {
+ eprintln!("Warning: Invalid include regex pattern '{}': {}", self.include, e);
+ return false;
+ }
};
if inc_reg.find(path).is_none() {
return false;
}
}
if !self.exclude.is_empty() {
let exc_reg = match Regex::new(&self.exclude) {
Ok(reg) => reg,
- Err(_) => return true,
+ Err(e) => {
+ eprintln!("Warning: Invalid exclude regex pattern '{}': {}", self.exclude, e);
+ return true;
+ }
};
if exc_reg.find(path).is_some() {
return false;
}
}
true
}Alternatively, validate regex patterns in the constructor and panic early with a clear error message.
| let result = match self.document_mode { | ||
| DocumentMode::Single | DocumentMode::Multi => { | ||
| serde_yaml::from_str::<serde_json::Value>(content)? | ||
| } |
There was a problem hiding this comment.
DocumentMode::Multi is not implemented.
Both Single and Multi document modes execute the same code path. Multi-document YAML files contain multiple documents separated by --- and typically parse to an array of values. The current implementation only parses a single document.
Either:
- Implement multi-document support using
serde_yaml::Deserializer::from_str()with iteration - Remove the
Multivariant and document that only single-document mode is supported - Return an error if
Multimode is configured
For reference, multi-document YAML parsing typically looks like:
DocumentMode::Multi => {
let deserializer = serde_yaml::Deserializer::from_str(content);
let documents: Result<Vec<_>, _> = deserializer
.map(|doc| serde_json::Value::deserialize(doc))
.collect();
serde_json::Value::Array(documents?)
}🤖 Prompt for AI Agents
In rust-plugins/yaml/src/lib.rs around lines 88 to 91, DocumentMode::Multi
currently falls through to the single-document path; implement real
multi-document parsing by using serde_yaml::Deserializer::from_str(content), map
each document to serde_json::Value via Value::deserialize(doc), collect into a
Result<Vec<_>, _>, and return serde_json::Value::Array(documents?) so multi-doc
YAML yields an array of documents; ensure errors are propagated with the
existing ? handling.
| if let serde_json::Value::Object(object) = result.clone() { | ||
| for (key, val) in object { | ||
| export_val.push_str(&format!("export var {} = {};\n", key, val)); | ||
| } | ||
| } |
There was a problem hiding this comment.
JavaScript identifier generation is unsafe.
Line 97 generates JavaScript code without validating that keys are valid identifiers. This will produce invalid JavaScript if:
- Keys contain spaces or special characters (e.g.,
"my-key","my key") - Keys are JavaScript reserved words (e.g.,
"class","function","return") - Keys start with numbers (e.g.,
"123key")
Additionally, line 95 clones the entire parsed value just to check if it's an object, which is inefficient.
Apply this diff to fix both issues:
let mut export_val = String::new();
- if let serde_json::Value::Object(object) = result.clone() {
+ if let serde_json::Value::Object(ref object) = result {
for (key, val) in object {
- export_val.push_str(&format!("export var {} = {};\n", key, val));
+ // Use bracket notation for safe property access
+ export_val.push_str(&format!("export const {} = {};\n",
+ serde_json::to_string(key).unwrap(), val));
}
}Or alternatively, validate identifiers and use dot notation only when safe:
fn is_valid_js_identifier(s: &str) -> bool {
// Check if it's a valid JS identifier and not a reserved word
// Simplified check - a full implementation would be more comprehensive
!s.is_empty()
&& s.chars().next().unwrap().is_alphabetic()
&& s.chars().all(|c| c.is_alphanumeric() || c == '_' || c == '$')
&& !["class", "function", "return", "var", "let", "const", /* ... */].contains(&s)
}
// Then in yaml_to_js:
if is_valid_js_identifier(key) {
export_val.push_str(&format!("export const {} = {};\n", key, val));
}| fn load( | ||
| &self, | ||
| param: &PluginLoadHookParam, | ||
| _context: &std::sync::Arc<farmfe_core::context::CompilationContext>, | ||
| _hook_context: &farmfe_core::plugin::PluginHookContext, | ||
| ) -> farmfe_core::error::Result<Option<PluginLoadHookResult>> { | ||
| if is_yaml_file(¶m.module_id) { | ||
| let content = read_to_string(param.resolved_path).unwrap(); | ||
| return Ok(Some(PluginLoadHookResult { | ||
| content, | ||
| source_map: None, | ||
| module_type: ModuleType::Custom(YAML_MODULE_TYPE.to_string()), | ||
| })); | ||
| } | ||
| Ok(None) | ||
| } |
There was a problem hiding this comment.
File read error causes panic instead of returning an error.
Line 117 uses .unwrap() on read_to_string(), which will panic if the file cannot be read (e.g., permission denied, file not found, IO error). This crashes the entire build process.
The load hook returns Result<Option<...>>, so IO errors should be propagated:
fn load(
&self,
param: &PluginLoadHookParam,
_context: &std::sync::Arc<farmfe_core::context::CompilationContext>,
_hook_context: &farmfe_core::plugin::PluginHookContext,
) -> farmfe_core::error::Result<Option<PluginLoadHookResult>> {
if is_yaml_file(¶m.module_id) {
- let content = read_to_string(param.resolved_path).unwrap();
+ let content = read_to_string(param.resolved_path)
+ .map_err(|e| farmfe_core::error::CompilationError::GenericError(
+ format!("Failed to read YAML file {}: {}", param.resolved_path, e)
+ ))?;
return Ok(Some(PluginLoadHookResult {
content,
source_map: None,
module_type: ModuleType::Custom(YAML_MODULE_TYPE.to_string()),
}));
}
Ok(None)
}🤖 Prompt for AI Agents
In rust-plugins/yaml/src/lib.rs around lines 110-125, the code uses
read_to_string(...).unwrap() which will panic on IO errors; replace the unwrap
with proper error propagation so the load hook returns a
farmfe_core::error::Result error instead of crashing: call
read_to_string(param.resolved_path) and propagate errors using the ? operator
(or map_err to convert the std::io::Error into the crate's farmfe_core::error
type) so the function returns Err on read failures, keeping the rest of the
returned PluginLoadHookResult logic unchanged.
| fn transform( | ||
| &self, | ||
| param: &PluginTransformHookParam, | ||
| _context: &std::sync::Arc<farmfe_core::context::CompilationContext>, | ||
| ) -> farmfe_core::error::Result<Option<PluginTransformHookResult>> { | ||
| if param.module_type != ModuleType::Custom(YAML_MODULE_TYPE.to_string()) { | ||
| return Ok(None); | ||
| } | ||
|
|
||
| if !self.should_process_path(param.resolved_path) { | ||
| return Ok(None); | ||
| } | ||
|
|
||
| let code = match self.yaml_to_js(¶m.content) { | ||
| Ok(code) => code, | ||
| Err(e) => panic!("Failed to parse YAML: {}", e), | ||
| }; | ||
|
|
||
| Ok(Some(PluginTransformHookResult { | ||
| content: code, | ||
| module_type: Some(ModuleType::Js), | ||
| source_map: None, | ||
| ignore_previous_source_map: false, | ||
| })) | ||
| } |
There was a problem hiding this comment.
YAML parsing error causes panic instead of returning an error.
Line 142 uses panic!() when YAML parsing fails. This crashes the entire build process. The transform hook returns Result<Option<...>>, so parsing errors should be propagated.
Apply this diff:
fn transform(
&self,
param: &PluginTransformHookParam,
_context: &std::sync::Arc<farmfe_core::context::CompilationContext>,
) -> farmfe_core::error::Result<Option<PluginTransformHookResult>> {
if param.module_type != ModuleType::Custom(YAML_MODULE_TYPE.to_string()) {
return Ok(None);
}
if !self.should_process_path(param.resolved_path) {
return Ok(None);
}
- let code = match self.yaml_to_js(¶m.content) {
- Ok(code) => code,
- Err(e) => panic!("Failed to parse YAML: {}", e),
- };
+ let code = self.yaml_to_js(¶m.content)
+ .map_err(|e| farmfe_core::error::CompilationError::GenericError(
+ format!("Failed to parse YAML file {}: {}", param.resolved_path, e)
+ ))?;
Ok(Some(PluginTransformHookResult {
content: code,
module_type: Some(ModuleType::Js),
source_map: None,
ignore_previous_source_map: false,
}))
}📝 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.
| fn transform( | |
| &self, | |
| param: &PluginTransformHookParam, | |
| _context: &std::sync::Arc<farmfe_core::context::CompilationContext>, | |
| ) -> farmfe_core::error::Result<Option<PluginTransformHookResult>> { | |
| if param.module_type != ModuleType::Custom(YAML_MODULE_TYPE.to_string()) { | |
| return Ok(None); | |
| } | |
| if !self.should_process_path(param.resolved_path) { | |
| return Ok(None); | |
| } | |
| let code = match self.yaml_to_js(¶m.content) { | |
| Ok(code) => code, | |
| Err(e) => panic!("Failed to parse YAML: {}", e), | |
| }; | |
| Ok(Some(PluginTransformHookResult { | |
| content: code, | |
| module_type: Some(ModuleType::Js), | |
| source_map: None, | |
| ignore_previous_source_map: false, | |
| })) | |
| } | |
| fn transform( | |
| &self, | |
| param: &PluginTransformHookParam, | |
| _context: &std::sync::Arc<farmfe_core::context::CompilationContext>, | |
| ) -> farmfe_core::error::Result<Option<PluginTransformHookResult>> { | |
| if param.module_type != ModuleType::Custom(YAML_MODULE_TYPE.to_string()) { | |
| return Ok(None); | |
| } | |
| if !self.should_process_path(param.resolved_path) { | |
| return Ok(None); | |
| } | |
| let code = self.yaml_to_js(¶m.content) | |
| .map_err(|e| farmfe_core::error::CompilationError::GenericError( | |
| format!("Failed to parse YAML file {}: {}", param.resolved_path, e) | |
| ))?; | |
| Ok(Some(PluginTransformHookResult { | |
| content: code, | |
| module_type: Some(ModuleType::Js), | |
| source_map: None, | |
| ignore_previous_source_map: false, | |
| })) | |
| } |
🤖 Prompt for AI Agents
In rust-plugins/yaml/src/lib.rs around lines 127 to 151, the YAML parse failure
currently calls panic!() which aborts the whole build; instead propagate the
parse error through the function's Result. Replace the panic! branch so that the
Err from yaml_to_js is converted into the crate's
farmfe_core::error::Result::Err (using map_err or match and returning Err(...))
with a clear context message (e.g., "Failed to parse YAML") so the transform
hook returns the error rather than panicking.
1 participant
Description:
move plugins from farm_plugins repo
BREAKING CHANGE:
None
Related issue (if exists):
None
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.