Skip to content

Conversation

@HigorAlves
Copy link
Contributor

@HigorAlves HigorAlves commented Dec 2, 2025

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
image

What's added in this PR?

Provide an implementation summary. What did you add beyond the described task? What are the additional details you'd like people to pay attention to? To help people review this PR understand what's changed.

Screenshots

If applicable, add some screenshots of the expected behavior.

What's the issues or discussion related to this PR ?

Provide some background information related to this PR, including issues or task. Prior to this PR what's the behavior that wasn't expected.

If there wasn't discussion related to this PR, you can include the reasoning behind this PR of why you did it.

What are the steps to test this PR?

To help reviewer and tester to understand what's needed

Documentation update for this PR (if applicable)?

Add documentation if how the application will behave differently than previous state. Copy paste your PR in zephyr-documentation PR link here.

(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

  • I have added/updated/opened a PR to documentation to cover this new behavior
  • I have added an explanation of my changes
  • I have written new tests (if applicable)
  • I have tested this locally (standing from a first time user point of view, never touch this app before)
  • I have/will run tests, or ask for help to add test

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
@HigorAlves HigorAlves self-assigned this Dec 2, 2025
@nx-cloud
Copy link

nx-cloud bot commented Dec 2, 2025

View your CI Pipeline Execution ↗ for commit c5c8c42

Command Status Duration Result
nx run e2e-deployment:e2e-test -- --passWithNoT... ✅ Succeeded 1s View ↗
nx affected -t test ✅ Succeeded 2m 54s View ↗
nx affected -t build --skipRemoteCache ✅ Succeeded 3m 11s View ↗
nx affected -t lint ✅ Succeeded 7s View ↗

☁️ Nx Cloud last updated this comment at 2025-12-09 17:50:00 UTC

@HigorAlves HigorAlves marked this pull request as ready for review December 3, 2025 14:09
@HigorAlves HigorAlves requested a review from Nsttt December 3, 2025 15:12
@ryok90
Copy link
Collaborator

ryok90 commented Dec 9, 2025

@claude please review

@claude
Copy link

claude bot commented Dec 9, 2025

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:


🎯 Summary

This PR introduces a new zephyr-metro-plugin package to support Metro/React Native applications with Module Federation, OTA updates, and Zephyr Cloud integration. The implementation is well-structured but has several critical issues that need to be addressed, particularly around documentation and API consistency.


🔴 Critical Issues

1. Conflicting API Implementations (libs/zephyr-metro-plugin/src/lib/)

There are TWO different implementations of withZephyr with incompatible APIs:

File 1: with-zephyr.ts (Lines 23-32)

export function withZephyr(zephyrOptions: ZephyrMetroOptions = {}) {
  return async (metroConfig: ConfigT): Promise<ConfigT> => { ... }
}
  • Takes a ZephyrMetroOptions object
  • Returns function that accepts Metro ConfigT
  • Modern, options-based API

File 2: withZephyr.ts (Lines 20-43)

export function withZephyr(
  context?: string,
  platform: 'ios' | 'android' = 'ios',
  mode: 'development' | 'production' = 'development',
  outDir = 'dist'
): (config: ...) => Promise<...> { ... }
  • Takes 4 positional parameters
  • Returns function that accepts Module Federation config
  • Legacy style API

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 src/index.ts exports from File 1, but File 2 is also present.

Recommendation:

  • Choose ONE API design and remove the other
  • If supporting both, clearly document which is deprecated
  • Use different function names if both are needed (e.g., withZephyrMetro vs withZephyrLegacy)

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  # ❌ Deprecated

Should use:

npx @react-native-community/cli init  # ✅ Current

Additional Issues in TESTING.md:

  • Line 46: Comment mentions "This is not metro, this is repack" - needs clarification
  • The documentation is overly verbose with generic information rather than practical step-by-step instructions
  • Missing the simple "1 host + 1 remote" example requested by @Nsttt

3. README.md API Examples Don't Match Implementation

The README shows this API (lines 52-78):

module.exports = withZephyr(
  __dirname,
  'ios',
  'production',
  'dist'
)(mfConfig);

But the actual exported API from src/index.ts is:

withZephyr({ name: 'MyApp', target: 'ios', remotes: {...} })(metroConfig)

This is a major documentation bug that will break user implementations.


⚠️ Major Issues

4. No Tests (libs/zephyr-metro-plugin/)

The PR adds zero test files despite adding 2,897 lines of code. This is a significant gap for production code:

  • No unit tests for core functionality
  • No integration tests for Metro config transformation
  • No tests for transformer injection logic
  • No tests for dependency resolution

5. Runtime Plugin Injection May Fail Silently (libs/zephyr-metro-plugin/src/lib/zephyr-transformer.ts:122-135)

The transformer injects code that require('zephyr-xpack-internal'), but:

  • This is a workspace dependency, not listed in package.json dependencies
  • Runtime require will likely fail in production
  • Error is only logged to console, not reported properly
try {
  const { createZephyrRuntimePlugin } = require('zephyr-xpack-internal');
  // ...
} catch (error) {
  console.warn('[Zephyr] Failed to initialize runtime plugin:', error);
}

Recommendation: Either add zephyr-xpack-internal as a peer dependency or bundle it.

6. Inconsistent Platform Handling (libs/zephyr-metro-plugin/src/lib/zephyr-metro-plugin.ts)

The ZephyrMetroPlugin class uses Platform type but stores it as platform in config. However, with-zephyr.ts doesn't have platform handling at all - it only uses the target from zephyrOptions.target which is optional.

This creates confusion about which implementation is the source of truth.

7. Missing Error Handling for Manifest Generation (libs/zephyr-metro-plugin/src/lib/with-zephyr.ts:129-144)

The generateManifestFile function catches errors but only logs them. If manifest generation fails:

  • Build continues without indication of failure
  • Runtime may break without the manifest
  • No way for users to know something went wrong

8. Package.json Points to Non-Built Source (libs/zephyr-metro-plugin/package.json:7-8)

"main": "./src/index.js",
"types": "./src/index.d.ts",

Should point to dist/ or build output, not source files. TypeScript files need to be compiled before publishing.


💡 Moderate Issues

9. Global Type Pollution (libs/zephyr-metro-plugin/src/lib/global.d.ts)

Adds types to global namespace without proper scoping. Consider namespace wrapping or more specific type guards.

10. Hardcoded Manifest Path (Multiple files)

The manifest URL is hardcoded to /zephyr-manifest.json in several places. Should be configurable via options.

11. Over-Broad File Targeting in Transformer (libs/zephyr-metro-plugin/src/lib/zephyr-transformer.ts:47-71)

The transformer targets files containing AppRegistry or loadRemote. This could incorrectly inject code into library files or tests that import these for other reasons.

Recommendation: Make targeting more conservative or add configuration options to specify entry files.

12. Missing ApplicationConsumes Export (libs/zephyr-edge-contract/src/index.ts)

The PR description mentions "Export ApplicationConsumes from zephyr-edge-contract" but I can see it's already exported in line 25. However, it's only exported as a type. Verify this is correct for the use case.

13. Inconsistent Logging (Multiple files)

Uses mix of ze_log, logFn, and console.log. Should standardize on one logging approach.


✅ Positive Aspects

  1. Well-structured architecture: Clear separation between config, transformer, and internal utilities
  2. Good TypeScript usage: Proper typing throughout most of the codebase
  3. Reuse of existing infrastructure: Leverages ZephyrEngine and other established patterns
  4. Module Federation integration: Proper handling of remotes and dependency resolution
  5. Build stats generation: Comprehensive tracking of build metadata
  6. Runtime plugin architecture: Clean separation of build-time and runtime concerns

📝 PR Description Issues

The PR description has many unfilled sections:

  • No implementation summary
  • No screenshots
  • No related issues/discussions
  • No testing steps
  • No documentation PR link (required by checklist)
  • No explanation of what's left to be done
  • No risk assessment

Given this is a major feature adding 2,897 lines, these sections should be filled out.


🔧 Recommendations

Immediate Actions Required:

  1. Resolve API conflict: Choose one withZephyr implementation and remove/rename the other
  2. Fix README.md: Update all examples to match the actual API
  3. Fix TESTING.md:
    • Remove deprecated commands
    • Add simple 1 host + 1 remote example as requested by @Nsttt
    • Make it practical and step-by-step
  4. Fix package.json: Point to build output, not source files
  5. Add dependency: Include zephyr-xpack-internal in dependencies or peerDependencies
  6. Add tests: At minimum, unit tests for core functions
  7. Fill out PR description: Complete all required sections

Follow-up Actions:

  1. Add error boundaries for critical paths (manifest generation, runtime injection)
  2. Make manifest URL configurable
  3. Standardize logging approach
  4. Add integration tests
  5. Consider making transformer file targeting configurable
  6. Add JSDoc comments to public APIs

🎓 Additional Notes

This is a solid foundation for Metro/React Native support, but the API inconsistency and documentation issues would cause significant user confusion. The lack of tests is concerning for code that will be used in production builds.

The reviewer @Nsttt's feedback about TESTING.md is accurate - it reads like AI-generated documentation with generic setup info rather than a practical testing guide.


Files Requiring Immediate Attention:

  1. libs/zephyr-metro-plugin/src/lib/with-zephyr.ts vs libs/zephyr-metro-plugin/src/lib/withZephyr.ts
  2. libs/zephyr-metro-plugin/README.md (API examples)
  3. libs/zephyr-metro-plugin/TESTING.md (deprecated commands, missing examples)
  4. libs/zephyr-metro-plugin/package.json (main/types paths, dependencies)
  5. PR description template (fill out all sections)

…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
@HigorAlves
Copy link
Contributor Author

@claude please review

@claude
Copy link

claude bot commented Dec 9, 2025

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:


🎯 Summary

This PR introduces a new zephyr-metro-plugin package to support Metro/React Native applications with Module Federation, OTA updates, and Zephyr Cloud integration. The implementation is well-structured overall with good TypeScript usage and clear separation of concerns. However, there are several issues that need to be addressed, particularly around documentation consistency and testing coverage.


🔴 Critical Issues

1. 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 npx react-native init

  • Should use: npx @react-native-community/cli init instead
  • This is the current React Native CLI standard

Overall structure: The document is more of a setup guide than a practical testing guide

  • Missing: Simple, step-by-step example for "1 host + 1 remote" as requested by @Nsttt
  • Issue: Contains verbose generic information rather than practical steps
  • Recommendation: Add a concise step-by-step section at the top showing exactly how to:
    1. Create host and remote apps
    2. Configure them with Zephyr
    3. Verify they work together
    4. Deploy them to Zephyr Cloud
    5. Confirm Zephyr integration is working

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 zephyr-xpack-internal:

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:

  • Runtime continues without the plugin
  • Only logs a warning in dev mode
  • No indication in production that features may not work

Recommendation: Consider either:

  • Adding clearer documentation about when zephyr-xpack-internal is required
  • Making it a required peer dependency if the runtime plugin is essential
  • Or keeping it optional but documenting what features won't work without it

⚠️ Major Issues

4. Test Coverage is Sparse (libs/zephyr-metro-plugin/src/lib/)

The PR adds 2,936 lines of code but only includes 2 test files:

  • with-zephyr.spec.ts - 114 lines (mostly unit tests of helper functions)
  • zephyr-transformer.spec.ts - 155 lines (mostly unit tests of helper functions)

Missing test coverage:

  • No tests for ZephyrMetroPlugin class (233 lines)
  • No tests for zephyr-metro-command-wrapper.ts (72 lines)
  • No integration tests for the full Metro config transformation
  • No tests for manifest generation
  • No tests for dependency resolution
  • Most internal utilities (extract-mf-remotes, mutate-mf-config, etc.) are untested

Impact: While the existing tests are well-written, the overall coverage is limited. This is a significant gap for production code that will be used in builds.

Recommendation: Add at least:

  • Integration tests for withZephyr() with actual Metro config
  • Tests for ZephyrMetroPlugin lifecycle methods
  • Tests for manifest generation and serving

5. Global Type Declarations Could Be More Specific (libs/zephyr-metro-plugin/src/lib/global.d.ts)

Lines 34-40 declare globals with unknown type:

var __ZEPHYR_RUNTIME_PLUGIN__: unknown | undefined;
var __ZEPHYR_RUNTIME_PLUGIN_INSTANCE__: unknown | undefined;

Issue: Using unknown makes these globals hard to work with in application code.

Recommendation: Either:

  • Import and use the actual runtime plugin type from zephyr-xpack-internal
  • Or define a proper interface for the runtime plugin in this package
  • Document why unknown is used if there's a circular dependency concern

6. Hardcoded Manifest Path in Multiple Places

The manifest URL /zephyr-manifest.json is used as a default in multiple files:

  • with-zephyr.ts:43
  • zephyr-transformer.ts:39

Current Status: The implementation DOES make it configurable via ZephyrMetroOptions.manifestPath

Minor concern: The path is passed through config properly, so this is actually handled well. No action needed.

7. Manifest Generation Failure Handling (libs/zephyr-metro-plugin/src/lib/with-zephyr.ts:116-126)

When manifest generation fails:

if (!manifestGenerated) {
  ze_log.error('Manifest file generation failed - runtime updates may not work correctly');
}

Issue: The build continues without throwing an error. If the manifest is critical:

  • Production builds might be broken without obvious indication
  • Users might not notice the error in build logs

Recommendation: Consider either:

  • Throwing an error if manifest generation fails
  • Or making it more obvious that this is a warning vs fatal error
  • Add a config option like failOnManifestError: boolean

8. Transformer File Targeting Could Be More Conservative (libs/zephyr-metro-plugin/src/lib/zephyr-transformer.ts:52-79)

The transformer targets files that:

  1. Match entry file patterns (configurable, good ✅)
  2. Contain AppRegistry.registerComponent or AppRegistry.runApplication

Concern: Criterion #2 could inject code into test files or library code that imports these for other reasons.

Current mitigation:

  • The implementation already has configurable entryFiles patterns ✅
  • Only targets files that actually register the app

Recommendation: This is actually pretty conservative already. The concern about over-broad targeting is minor. Maybe add a note in the README about how targeting works.


💡 Moderate Issues

9. PR Description Template Not Filled Out

The PR description has many empty sections:

  • ❌ No implementation summary beyond bullet points
  • ❌ No screenshots
  • ❌ No related issues/discussions
  • ❌ No detailed testing steps
  • ❌ No documentation PR link (required by checklist)
  • ❌ No explanation of what's left to be done
  • ❌ No risk assessment

Recommendation: Fill out these sections to help reviewers understand:

  • What changed beyond the basic feature list
  • How to test this manually
  • What risks might exist
  • Link to any documentation updates

10. Inconsistent Logging Approaches (Multiple files)

The codebase uses multiple logging approaches:

  • ze_log.error(), ze_log.app(), ze_log.config(), ze_log.manifest() from zephyr-agent
  • console.log() in generated runtime code (zephyr-transformer.ts:128-133)

Current Status: The mix is actually reasonable:

  • ze_log.* for build-time logging ✅
  • console.log/warn for runtime (in the browser/RN app) ✅

Minor concern: The generated runtime code uses console.log which is appropriate for the React Native runtime environment. This is fine.

11. Module Federation Config Structure Needs Clarification (README.md:84-128)

The README shows Module Federation setup but notes (line 118-119):

// Note: Module Federation "exposes" configuration is handled separately
// through @callstack/repack or similar Metro MF solutions

Issue: It's not immediately clear:

  • How to actually configure the exposes part
  • What additional packages are needed
  • Whether @callstack/repack is required or optional

Recommendation: Add a section explaining:

  • The relationship between this plugin and Re.Pack
  • How to configure exposes
  • Complete end-to-end example linking to working code

12. ApplicationConsumes Export Change (libs/zephyr-edge-contract/src/index.ts:24-27)

The PR correctly exports ApplicationConsumes as a type:

export type {
  ApplicationConsumes,
  ZephyrBuildStats,
  ZephyrDependency,
} from './lib/zephyr-build-stats';

✅ This is used correctly in zephyr-metro-plugin.ts:4 as an import type. No issues here.


✅ Positive Aspects

  1. Well-structured architecture: Clear separation between:

    • Config transformation (with-zephyr.ts)
    • Code transformation (zephyr-transformer.ts)
    • Build lifecycle management (zephyr-metro-plugin.ts)
    • Internal utilities (clean internal/ directory)
  2. Good TypeScript usage:

    • Proper typing throughout
    • Good use of interfaces for options
    • Type-safe function signatures
  3. Proper peer dependency handling:

    • Metro and React Native as peer dependencies ✅
    • zephyr-xpack-internal as optional peer dependency ✅
    • Workspace dependencies properly configured
  4. Reuse of existing infrastructure:

    • Leverages ZephyrEngine consistently
    • Uses established zephyr-agent patterns
    • Integrates with existing build stats system
  5. Runtime plugin architecture:

    • Clean separation of build-time and runtime concerns
    • Proper global namespace management to prevent double-initialization
    • Good error handling in injected runtime code
  6. Configurable and extensible:

    • Manifest path configurable ✅
    • Entry files configurable ✅
    • Platform support (iOS/Android) ✅
    • Good defaults with override options
  7. Documentation effort:

    • Comprehensive README with examples
    • API reference section
    • Troubleshooting guide
    • Testing guide (needs updates but shows effort)
  8. Proper package.json setup:

    • Correct main/types/exports configuration
    • Good keywords for discoverability
    • Proper repository and license info

🔧 Recommendations Priority

High Priority (Address before merge):

  1. Fix TESTING.md (Critical ❌):

    • Remove deprecated npx react-native init command
    • Add simple 1 host + 1 remote step-by-step example as requested by @Nsttt
    • Make it practical and action-oriented
  2. Add more tests (Major ⚠️):

    • At minimum: integration tests for withZephyr() transformation
    • Tests for ZephyrMetroPlugin lifecycle
    • Tests for manifest generation
  3. Fill out PR description (Moderate 💡):

    • Add implementation summary
    • Add testing steps
    • Note what's left to be done (if anything)
    • Add risk assessment

Medium Priority (Address soon):

  1. Improve global types (Major ⚠️):

    • Replace unknown with proper types for __ZEPHYR_RUNTIME_PLUGIN__
  2. Clarify Module Federation setup (Moderate 💡):

    • Add section on Re.Pack integration
    • Explain how to configure exposes
    • Link to complete examples
  3. Document runtime plugin dependency (Critical info, Major ⚠️):

    • Clarify when zephyr-xpack-internal is needed
    • Document what features require it
    • Add setup instructions for including it

Low Priority (Nice to have):

  1. Consider manifest error handling (Major ⚠️):

    • Add option to fail build on manifest error
    • Or make warning more prominent
  2. Add more test coverage over time:

    • Internal utilities
    • Edge cases
    • Error scenarios

📊 Code Quality Metrics

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:

  1. Documentation accuracy (TESTING.md deprecated commands)
  2. Test coverage (needs more comprehensive tests)
  3. 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:

  1. libs/zephyr-metro-plugin/TESTING.md - Remove deprecated commands, add practical example
  2. libs/zephyr-metro-plugin/src/lib/ - Add more test coverage
  3. libs/zephyr-metro-plugin/src/lib/global.d.ts - Improve type definitions
  4. PR description - Fill out template sections
  5. 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.
@HigorAlves HigorAlves force-pushed the feat/metro-plugin-v2 branch from 128dbc3 to 767c3a1 Compare December 9, 2025 17:14
…directory

- Moved integration and unit test files for better organization.
- Updated imports to reflect new directory structure.
@HigorAlves HigorAlves merged commit 2b74de8 into main Dec 9, 2025
11 checks passed
@HigorAlves HigorAlves deleted the feat/metro-plugin-v2 branch December 9, 2025 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants