-
Notifications
You must be signed in to change notification settings - Fork 12
feat(zephyr-metro-plugin): add Metro bundler plugin for React Native #270
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
Add new zephyr-metro-plugin package for Metro/React Native support: - Metro config integration via withZephyr() wrapper - Metro transformer for runtime plugin injection - Module Federation support for React Native - Build stats generation for Zephyr Cloud integration - Manifest endpoint for runtime dependency resolution Also includes: - Add 'metro' builder type to ZephyrEngine - Export ApplicationConsumes from zephyr-edge-contract - Add web runtime plugin export to zephyr-xpack-internal
|
View your CI Pipeline Execution ↗ for commit c5c8c42
☁️ Nx Cloud last updated this comment at |
|
@claude please review |
|
Claude finished @ryok90's task in 2m 21s —— View job Code Review Complete ✅I've conducted a comprehensive review of this PR which adds Metro bundler support for React Native. Here's my detailed feedback: 🎯 SummaryThis PR introduces a new 🔴 Critical Issues1. Conflicting API Implementations (libs/zephyr-metro-plugin/src/lib/)There are TWO different implementations of File 1: export function withZephyr(zephyrOptions: ZephyrMetroOptions = {}) {
return async (metroConfig: ConfigT): Promise<ConfigT> => { ... }
}
File 2: export function withZephyr(
context?: string,
platform: 'ios' | 'android' = 'ios',
mode: 'development' | 'production' = 'development',
outDir = 'dist'
): (config: ...) => Promise<...> { ... }
Impact: This is extremely confusing for users. The README.md shows the legacy API (File 2) while TESTING.md shows the modern API (File 1). The main export in Recommendation:
2. TESTING.md Contains Deprecated Commands (libs/zephyr-metro-plugin/TESTING.md:54)As noted by @Nsttt's review comment, the guide uses deprecated React Native commands: npx react-native init ZephyrMetroTest # ❌ DeprecatedShould use: npx @react-native-community/cli init # ✅ CurrentAdditional Issues in TESTING.md:
3. README.md API Examples Don't Match ImplementationThe README shows this API (lines 52-78): module.exports = withZephyr(
__dirname,
'ios',
'production',
'dist'
)(mfConfig);But the actual exported API from withZephyr({ name: 'MyApp', target: 'ios', remotes: {...} })(metroConfig)This is a major documentation bug that will break user implementations.
|
…unit tests - Removed old `withZephyr` implementation - Added unit tests for Metro remotes, transformer injection, and runtime behavior - Enhanced global typings for runtime plugin - Updated plugin to support configurable manifest paths and custom entry file patterns
|
@claude please review |
|
Claude finished @HigorAlves's task in 2m 26s —— View job Code Review Complete ✅I've conducted a comprehensive review of this PR which adds Metro bundler support for React Native. Here's my detailed feedback: 🎯 SummaryThis PR introduces a new 🔴 Critical Issues1. TESTING.md Needs Update Per Review Feedback (libs/zephyr-metro-plugin/TESTING.md)@Nsttt's review feedback is accurate - the guide needs improvement: Lines 20, 56: Uses deprecated command
Overall structure: The document is more of a setup guide than a practical testing guide
2. Package.json Points to dist/ But README Examples May Confuse (package.json:7-8)The package.json correctly points to build output: "main": "dist/index.js",
"types": "dist/index.d.ts",This is ✅ CORRECT - the previous review comment about this was inaccurate. However, the README.md shows examples that are consistent with the exported API, which is good. 3. Runtime Plugin Dependency Handling (libs/zephyr-metro-plugin/src/lib/zephyr-transformer.ts:118)The transformer injects code requiring var createZephyrRuntimePlugin = require('zephyr-xpack-internal').createZephyrRuntimePlugin;Current Status: This is marked as an optional peerDependency in package.json:26-28, which is reasonable. Issue: The injected code has proper try/catch error handling (lines 117-136), but if the dependency isn't available:
Recommendation: Consider either:
|
| Aspect | Rating | Notes |
|---|---|---|
| Architecture | ⭐⭐⭐⭐⭐ | Excellent separation of concerns |
| TypeScript Usage | ⭐⭐⭐⭐ | Good typing, minor improvements possible |
| Error Handling | ⭐⭐⭐⭐ | Good try/catch usage, some edge cases |
| Test Coverage | ⭐⭐ | Sparse - major gap |
| Documentation | ⭐⭐⭐⭐ | Good README, TESTING.md needs fixes |
| Integration | ⭐⭐⭐⭐⭐ | Excellent reuse of existing patterns |
Overall Assessment: 🟡 Solid foundation with documentation and testing gaps
🎓 Additional Notes
This is a well-architected addition to the Zephyr ecosystem. The core implementation is solid and follows established patterns. The main concerns are:
- Documentation accuracy (TESTING.md deprecated commands)
- Test coverage (needs more comprehensive tests)
- PR description completeness (helps reviewers)
The implementation itself is production-ready from an architecture standpoint. The issues are mostly around polish, testing, and documentation clarity.
The review feedback from @Nsttt about TESTING.md is spot-on - it needs to be more practical and use current commands.
Files Requiring Attention:
- libs/zephyr-metro-plugin/TESTING.md - Remove deprecated commands, add practical example
- libs/zephyr-metro-plugin/src/lib/ - Add more test coverage
- libs/zephyr-metro-plugin/src/lib/global.d.ts - Improve type definitions
- PR description - Fill out template sections
- libs/zephyr-metro-plugin/README.md - Clarify Module Federation setup
…nd improve error handling - Refactored manifest middleware to simplify logic and ensure base middleware is respected. - Improved resilience by explicitly handling manifest requests and errors. - Updated comments to clarify global declaration usage.
…Metro plugin - Added comprehensive tests for `withZephyr` Metro integration. - Introduced unit tests for `zephyrCommandWrapper` and `ZephyrMetroPlugin` class. - Enhanced global typings to support runtime plugin options and manifest updates.
128dbc3 to
767c3a1
Compare
…directory - Moved integration and unit test files for better organization. - Updated imports to reflect new directory structure.
Add new zephyr-metro-plugin package for Metro/React Native support:
Also includes:
What's added in this PR?
Screenshots
What's the issues or discussion related to this PR ?
What are the steps to test this PR?
Documentation update for this PR (if applicable)?
(Optional) What's left to be done for this PR?
(Optional) What's the potential risk and how to mitigate it?
(Required) Pre-PR/Merge checklist