-
Notifications
You must be signed in to change notification settings - Fork 8
fix(rust-plugins): invalid worker extension #141
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughIntroduces a patch release for @farmfe/plugin-worker: updates changeset metadata, bumps version, adds changelog entry, and modifies worker output behavior by setting entry_filename in OutputConfig and changing the resource lookup key from "<full_file_name>.mjs" to "<full_file_name>". Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Builder as Builder Config
participant Compiler as Worker Compiler
participant Resources as Resource Map
Dev->>Builder: Initialize Config
Note right of Builder: Set OutputConfig.entry_filename
Builder->>Compiler: Build with OutputConfig
Compiler->>Resources: Get("<full_file_name>")
alt Resource found
Resources-->>Compiler: Worker resource
Compiler-->>Dev: Emit output with correct entry filename
else Resource missing
Resources-->>Compiler: Not found
Compiler-->>Dev: Error: resource not found
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust-plugins/worker/src/lib.rs (1)
171-182: Inline worker cache key bug causes panic.Cache is populated with key file_name, but the inline branch reads with resolved_path, so get(...).unwrap() will panic whenever ?inline is used.
Apply this fix:
- let content_bytes = worker_cache.get(resolved_path).unwrap(); + let content_bytes = worker_cache + .get(&file_name) + .unwrap_or_else(|| { + // Fallback: build and insert if somehow missing + insert_worker_cache(&worker_cache, file_name.clone(), build_worker(resolved_path, module_id, compiler_config, host_config)) + });Also applies to: 209-213
🧹 Nitpick comments (4)
rust-plugins/worker/src/lib.rs (4)
91-93: Resource lookup by computed filename is correct; avoid hard panics.Using full_file_name fixes the .mjs issue. However, unwrap() will panic if the resource isn’t present. Prefer propagating an error.
Apply this minimal improvement (or wire through a Result if you prefer):
- let resource = resources_map.get(&full_file_name).unwrap(); + let resource = resources_map + .get(&full_file_name) + .unwrap_or_else(|| panic!("worker resource not found: {}", full_file_name));
116-121: Robust extension parsing (multi-dot names, no extension).split_once(".") breaks on multi-dot filenames and panics on dotless names. Use rsplit_once with a safe fallback.
- let (file_name, ext) = file_name_ext.split_once(".").unwrap(); + let (name_stem, ext) = match file_name_ext.rsplit_once('.') { + Some((stem, ext)) => (stem, ext), + None => (file_name_ext.as_str(), "js"), + };Also update the TransformOutputFileNameParams below to use name_stem.
122-129: Hash input comment vs. implementation mismatch.Comment says “resolved_path + file_name_ext” but bytes uses module_id. Either update the comment or include resolved_path to ensure per-path uniqueness.
- bytes: &module_id.as_bytes(), + bytes: format!("{}|{}", resolved_path, module_id).as_bytes(),
133-137: Handle more TS extensions.Only .ts is normalized. Add .tsx (and optionally .mts/.cts) to avoid invalid runtime extensions.
- let file_name = if file_name.ends_with(".ts") { - file_name.replace(".ts", ".js") - } else { - file_name - }; + let file_name = if file_name.ends_with(".tsx") { + file_name.trim_end_matches(".tsx").to_string() + ".js" + } else if file_name.ends_with(".ts") { + file_name.trim_end_matches(".ts").to_string() + ".js" + } else { + file_name + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/late-moles-decide.md(1 hunks).changeset/pre.json(1 hunks)rust-plugins/worker/CHANGELOG.md(1 hunks)rust-plugins/worker/package.json(1 hunks)rust-plugins/worker/src/lib.rs(3 hunks)
⏰ 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). (8)
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm64-gnu
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm64-musl
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-ia32-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - darwin-x64
- 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-x64-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-x64-gnu
🔇 Additional comments (5)
rust-plugins/worker/package.json (1)
3-3: Version bump aligns with patch fix; ensure tags/publication.Looks good. Please confirm a publish will be cut under the beta tag and that CHANGELOG/changeset are in sync.
rust-plugins/worker/CHANGELOG.md (1)
3-8: Changelog entry is clear.Concise and matches the code changes.
.changeset/late-moles-decide.md (1)
1-5: Changeset LGTM.Scope and summary match the patch intent.
.changeset/pre.json (1)
33-34: Pre-mode entry added correctly.The new changeset id is included; ordering is fine.
rust-plugins/worker/src/lib.rs (1)
74-82: Setting OutputConfig.entry_filename fixes the extension/key mismatch.Good call—this ensures the emitted resource key matches the computed filename.
Please run a local build with a worker entry whose source ends with .ts and confirm the emitted key equals the computed file_name (no extra .mjs).
Summary by CodeRabbit