feat: split into optional dependencies #134
Conversation
WalkthroughImplements per-platform npm package publishing for mobilecli across Darwin and Linux architectures (ARM64 and x64) plus Windows x64. Introduces platform-specific package.json files and updates the build workflow and main package to support optional dependencies and dynamic binary resolution. Changes
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
publish/npm/package.json (1)
13-20: Consider how you want platform package versions to track the wrapper
optionalDependenciesare pinned to"0.0.4"while the publish workflow runsnpm version "${{ github.ref_name }}"in both the wrapper and each per-platform package. That means future wrapper releases will keep depending on the exact0.0.4binary packages unless this file is also updated.If you intend the wrapper to consume newer binary package versions without manually editing this file every time, consider:
- Using a semver range (e.g.
^0.0.4) instead of a fixed version, or- Adding a small pre-publish step that rewrites these versions to
${{ github.ref_name }}alongsidenpm version..github/workflows/build.yml (1)
399-447: Platform-specific npm publish logic is correct but quite repetitiveThe copy/chmod/
npm version/npm publishsequence for each platform looks consistent with the directory layout and package names, so behavior-wise this should work.If you expect to add more platforms or tweak the flow often, consider:
- Moving this into a small matrix job, or
- Using a loop over a list of
(platform, package-dir, binary-name)tuples inside the script,to reduce duplication and lower the chance of one platform block drifting from the others.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/build.yml(1 hunks)publish/npm-darwin-amd64/package.json(1 hunks)publish/npm-darwin-arm64/package.json(1 hunks)publish/npm-linux-amd64/package.json(1 hunks)publish/npm-linux-arm64/package.json(1 hunks)publish/npm-windows-amd64/package.json(1 hunks)publish/npm/index.js(2 hunks)publish/npm/package.json(1 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). (2)
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
publish/npm-windows-amd64/package.json (1)
1-13: Windows x64 binary manifest looks consistentName, version, and
os/cpuconstraints match the intendedwin32x64 binary package. No issues spotted.publish/npm-darwin-amd64/package.json (1)
1-13: macOS x64 binary manifest is aligned with the per-platform schemePackage name, description, and
os/cpuselectors correctly target macOS x64 and match the naming used in the wrapper.publish/npm-darwin-arm64/package.json (1)
1-13: macOS ARM64 manifest matches naming and platform constraintsThe
@mobilenext/mobilecli-darwin-arm64package is correctly scoped toos: ["darwin"]andcpu: ["arm64"], matching the build artifacts and index.js mappings.publish/npm-linux-arm64/package.json (1)
1-13: Linux ARM64 manifest is consistent with the other platform packagesName, description, and
os/cpuvalues correctly target Linux ARM64 and line up with the binary naming used in the workflow and wrapper.publish/npm-linux-amd64/package.json (1)
1-13: Linux x64 manifest looks correctThe
@mobilenext/mobilecli-linux-amd64package correctly targetsos: ["linux"],cpu: ["x64"]and matches the naming used elsewhere.publish/npm/index.js (1)
9-53: Platform/arch → package mapping is clear and well-boundedThe switch on
process.platform/process.archmaps cleanly to the new per-platform package names and binary filenames, and the extraif (!packageName)guard gives a clear error for unsupported combinations (e.g., non-x64 Windows or non-arm64/x64 Linux).This part looks solid.
| let binaryPath; | ||
| try { | ||
| const packagePath = require.resolve(packageName); | ||
| binaryPath = join(packagePath, "..", binaryName); | ||
| } catch (error) { | ||
| console.error(`Failed to find ${packageName}. Please reinstall @mobilenext/mobilecli.`); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and read the relevant section
cat -n publish/npm/index.js | head -70 | tail -20Repository: mobile-next/mobilecli
Length of output: 733
🏁 Script executed:
# Let's also look for package.json files to understand the package structure
find . -name "package.json" -type f | head -20Repository: mobile-next/mobilecli
Length of output: 310
🏁 Script executed:
# Check the project root package.json to understand the package structure
cat package.json | head -50Repository: mobile-next/mobilecli
Length of output: 109
🏁 Script executed:
# Let's examine the platform-specific package.json files to understand their structure
echo "=== npm-darwin-amd64 package.json ===" && cat publish/npm-darwin-amd64/package.json
echo -e "\n=== npm-linux-amd64 package.json ===" && cat publish/npm-linux-amd64/package.jsonRepository: mobile-next/mobilecli
Length of output: 738
🏁 Script executed:
# Let's check the structure of these platform-specific package directories
ls -la publish/npm-darwin-amd64/ | head -20Repository: mobile-next/mobilecli
Length of output: 234
🏁 Script executed:
# Find where packageName is assigned/determined in the file
cat -n publish/npm/index.js | head -60Repository: mobile-next/mobilecli
Length of output: 1913
🏁 Script executed:
# Check if binary files exist in the platform directories
find publish/npm-darwin-amd64/ publish/npm-linux-amd64/ -type fRepository: mobile-next/mobilecli
Length of output: 139
🏁 Script executed:
# Let's check the main npm package.json that depends on these
cat publish/npm/package.jsonRepository: mobile-next/mobilecli
Length of output: 698
🏁 Script executed:
# Create a test to understand how require.resolve behaves with these packages
python3 << 'EOF'
# We need to understand Node.js require.resolve behavior
# Let's create a simple test scenario
# Key question: Does require.resolve(packageName) work without "main" in package.json?
# And does require.resolve(`${packageName}/package.json`) work?
# Based on Node.js documentation:
# - require.resolve() without a "main" field will fail
# - require.resolve('pkg/package.json') explicitly resolves the package.json
print("Testing Node.js require.resolve behavior:\n")
print("Scenario: Package has no 'main' field in package.json")
print("- require.resolve('packageName') -> Would FAIL (no main entry)")
print("- require.resolve('packageName/package.json') -> Would SUCCEED (explicit path)")
print("\nThe review comment's claim is CORRECT:")
print("These binary-only packages don't define 'main' in package.json")
print("So require.resolve(packageName) WILL throw an error")
print("Using require.resolve(`${packageName}/package.json`) is the correct fix")
EOFRepository: mobile-next/mobilecli
Length of output: 530
🏁 Script executed:
# Let's verify the actual binary file is in the right location
ls -la publish/npm-darwin-amd64/ | grep -E "mobilecli|bin"Repository: mobile-next/mobilecli
Length of output: 47
🏁 Script executed:
# Check if binary files actually exist in node_modules or if they're supposed to be there
# Let's look at a complete view of what should be in these packages
ls -la publish/npm-darwin-arm64/Repository: mobile-next/mobilecli
Length of output: 234
require.resolve(packageName) will fail for these binary-only packages because they lack a main entry point
The platform-specific packages (@mobilenext/mobilecli-darwin-amd64, @mobilenext/mobilecli-linux-amd64, etc.) only define a package.json with no main field and contain no JavaScript files. Node's require.resolve() requires a resolvable entry point (like main, index.js, or exports), so it will throw an error for these packages. This causes binaryPath to never be set and users will always encounter the "Failed to find" error.
Use require.resolve() with an explicit path to package.json instead:
let binaryPath;
try {
- const packagePath = require.resolve(packageName);
- binaryPath = join(packagePath, "..", binaryName);
+ const packageJsonPath = require.resolve(`${packageName}/package.json`);
+ binaryPath = join(packageJsonPath, "..", binaryName);
} catch (error) {
console.error(`Failed to find ${packageName}. Please reinstall @mobilenext/mobilecli.`);
process.exit(1);
}This resolves package.json directly (which is guaranteed to exist) and derives the correct binary directory path from there.
📝 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.
| let binaryPath; | |
| try { | |
| const packagePath = require.resolve(packageName); | |
| binaryPath = join(packagePath, "..", binaryName); | |
| } catch (error) { | |
| console.error(`Failed to find ${packageName}. Please reinstall @mobilenext/mobilecli.`); | |
| process.exit(1); | |
| } | |
| let binaryPath; | |
| try { | |
| const packageJsonPath = require.resolve(`${packageName}/package.json`); | |
| binaryPath = join(packageJsonPath, "..", binaryName); | |
| } catch (error) { | |
| console.error(`Failed to find ${packageName}. Please reinstall @mobilenext/mobilecli.`); | |
| process.exit(1); | |
| } |
🤖 Prompt for AI Agents
In publish/npm/index.js around lines 55 to 62, require.resolve(packageName) will
throw for binary-only packages without a main entry; change the resolve target
to package.json (e.g. require.resolve(packageName + '/package.json')) and derive
binaryPath by taking the directory of that resolved package.json and joining it
with the binary name, so binaryPath is correctly set for platform-specific
packages.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.