Skip to content

feat: adds release mode docs to repack doc #108

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

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

Thegrep01
Copy link
Contributor

@Thegrep01 Thegrep01 commented May 5, 2025

What's added in this PR?

Adds guide how to run app with repack in release mode

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

[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.]

Feature related update for this PR (if applicable)?

(Optional) What's left to be done for this PR?

Who do you wish to review this PR other than required reviewers?

(Required) Pre-PR/Merge checklist

  • I have added/updated our feature to sync with this PR
  • I have added an explanation of my changes
  • I have/will run tests, or ask for help to add test

Copy link

alwaysmeticulous bot commented May 5, 2025

🤖 No test run has been triggered as your Meticulous project has been deactivated (since you haven't viewed any test results in a while). Click here to reactivate.

Last updated for commit b68c019. This comment will update as new commits are pushed.

Copy link
Contributor

@zmzlois zmzlois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this whole block to the main /repack-mf.mdx so it's not hidden away on secondary menu

Should see something related to Releasing here
Screenshot 2025-05-07 at 15 19 11


{/* TODO: set ZC for iOS */}
{/* - For iOS, you can use the following command: */}
Copy link
Contributor

@zmzlois zmzlois May 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to fix this

- Bundle the MiniApp:

```bash
ZC=1 pnpm --filter MiniApp bundle
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ZC=1 pnpm --filter MiniApp bundle
## bundle towards ios
ZC=1 pnpm --filter MiniApp bundle:ios
## bundle towards android
ZC=1 pnpm --filter MiniApp bundle:android

to help us isolate platform deploy

- Bundle the HostApp:

```bash
ZC=1 pnpm --filter HostApp bundle
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ZC=1 pnpm --filter HostApp bundle
## bundle towards ios
ZC=1 pnpm --filter HostApp bundle:ios
## bundle towards android
ZC=1 pnpm --filter HostApp bundle:android

ZC=1 pnpm --filter HostApp bundle
```

- Command run with `ZC=1` will upload the bundles to Zephyr Cloud.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Command run with `ZC=1` will upload the bundles to Zephyr Cloud.
- Command run with `ZC=1` will deploy the bundles to Zephyr Cloud.

Copy link

claude bot commented Jun 16, 2025

Claude finished @Thegrep01's task —— View job


PR Review: Release Mode Documentation for Repack

Todo List:

  • Read changed files to understand documentation additions
  • Analyze technical accuracy and clarity of instructions
  • Review security implications of documented configurations
  • Check consistency with existing documentation patterns
  • Provide comprehensive review feedback with severity table
  • Generate overall quality score and recommendations

Purpose Summary: This PR adds comprehensive documentation explaining how to configure and run React Native applications with Re.Pack in production/release mode using Zephyr Cloud for bundle deployment.

Review Findings

Severity Line(s) Issue Recommendation
High docs/recipes/repack-mf.mdx:142 Hardcoded localhost URLs in production config Replace with environment variables or dynamic resolution
High docs/recipes/repack-mf.mdx:293-294 Incomplete iOS release build documentation Complete the iOS release build section or remove TODO
Medium docs/recipes/repack-mf.mdx:244-246 Unclear ZC environment variable explanation Add clearer explanation of when and why to use ZC=1
Medium docs/recipes/repack-mf.mdx:96,172 Inconsistent code block syntax highlighting Use consistent language identifiers (js vs javascript)
Medium docs/recipes/repack-mf.mdx:105,181 Potential environment variable collision Consider more descriptive env var names than just ZC
Low docs/recipes/repack-mf.mdx:167,240 Code formatting inconsistency Align indentation and spacing consistently
Low docs/recipes/repack-mf.mdx:282 Inconsistent terminology Use consistent terms: "upload" vs "deploy"
**Detailed Analysis & Recommendations**

Critical Issues

  1. Hardcoded URLs in Production Config

    // Current - problematic for production
    remotes: {
      MiniApp: `MiniApp@http://localhost:9001/${platform}/MiniApp.container.js.bundle`,
    }

    Fix: Document proper environment-based configuration:

    const getRemoteUrl = (appName, platform) => {
      if (process.env.ZC) {
        // Zephyr plugin will resolve this automatically
        return `${appName}@zephyr://dynamic-resolution`;
      }
      return `${appName}@http://localhost:9001/${platform}/${appName}.container.js.bundle`;
    };
    
    remotes: {
      MiniApp: getRemoteUrl('MiniApp', platform),
    }
  2. Incomplete iOS Documentation

    • Line 293-294 has a TODO comment for iOS release builds
    • This creates incomplete user experience
    • Either complete the documentation or clearly state current limitations

Documentation Quality Issues

  1. Environment Variable Clarity

    • The ZC=1 usage needs better explanation
    • Users need to understand the difference between development and release modes
    • Add a clear decision tree or flow chart
  2. Configuration Structure

    • The conditional withZephyr() wrapping could be confusing
    • Consider showing both configurations side-by-side for clarity

Security Considerations

  1. Environment Variable Exposure
    • Ensure sensitive Zephyr credentials aren't logged
    • Document proper secret management practices
    • Add warnings about not committing credentials

Code Quality Improvements

  1. Error Handling

    • Add error handling examples for failed bundle deployments
    • Document troubleshooting steps for common issues
  2. Validation

    • Add steps to verify successful deployment
    • Include commands to check bundle availability

Positive Aspects

Comprehensive Coverage: The documentation covers the complete flow from configuration to deployment
Practical Examples: Real-world configuration examples with proper context
Clear Structure: Well-organized sections with logical progression
Platform Support: Addresses both iOS and Android configurations
Integration Focus: Shows proper integration with existing Re.Pack workflow

Recommendations for Improvement

  1. Complete the iOS section or clearly document current limitations
  2. Add environment variable best practices section
  3. Include troubleshooting guide for common deployment issues
  4. Add validation steps to verify successful configuration
  5. Consider adding diagrams to illustrate the deployment flow

Overall Quality Score: 7/10

Top 3 Next Steps:

  1. Complete iOS release documentation - removes incomplete user experience
  2. Fix hardcoded localhost URLs - critical for production security
  3. Clarify ZC environment variable usage - improves user understanding

The documentation provides solid value but needs completion of critical sections and security improvements before merge.


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.

2 participants