feat: modernize mobile sdk build pipeline #877
Conversation
|
Warning Rate limit exceeded@transphorm has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughRestructures mobile-sdk-alpha packaging to a dual ESM/CJS layout. Updates exports in package.json, adds a browser subpath export, introduces tsup multi-target builds, adds a CJS tsconfig, and implements a post-build script that writes distribution package.json files and generates Metro shims. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant NPM as Package Scripts
participant TSUP as tsup (ESM+CJS)
participant PB as postBuild.mjs
participant FS as File System
Dev->>NPM: yarn build
NPM->>FS: rm -rf dist
NPM->>TSUP: build ESM (types) and CJS
TSUP->>FS: emit dist/esm/* and dist/cjs/*
NPM->>PB: yarn postbuild
PB->>FS: write dist/esm/package.json (module)
PB->>FS: write dist/cjs/package.json (commonjs)
PB->>FS: write dist/package.json (name, version, exports)
PB->>FS: create Metro shims from shimConfigs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
transphorm
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/mobile-sdk-alpha/scripts/postBuild.mjs (1)
37-44: Consider using template literals for better readability.The string concatenation for shim content could be more readable with template literals.
- writeFileSync( - path.join(shimDir, 'index.js'), - `// Shim file to help Metro resolve @selfxyz/mobile-sdk-alpha/${name}\nmodule.exports = require('${cjsTargetPath}');`, - ); - writeFileSync( - path.join(shimDir, 'index.d.ts'), - `// Shim file to help Metro resolve @selfxyz/mobile-sdk-alpha/${name} types\nexport * from "${dtsTarget}";`, - ); + writeFileSync( + path.join(shimDir, 'index.js'), + `// Shim file to help Metro resolve @selfxyz/mobile-sdk-alpha/${name} +module.exports = require('${cjsTargetPath}');` + ); + writeFileSync( + path.join(shimDir, 'index.d.ts'), + `// Shim file to help Metro resolve @selfxyz/mobile-sdk-alpha/${name} types +export * from "${dtsTarget}";` + );packages/mobile-sdk-alpha/package.json (1)
37-37: Consider adding error handling to the postbuild script.While the postbuild script integration looks good, consider what happens if the post-build step fails. The build process might appear successful even if shim generation fails.
Consider updating the build script to handle post-build failures:
- "build": "rm -rf dist && tsup && yarn postbuild", + "build": "rm -rf dist && tsup && yarn postbuild || (echo 'Post-build failed' && exit 1)",
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/mobile-sdk-alpha/package.json(1 hunks)packages/mobile-sdk-alpha/scripts/postBuild.mjs(1 hunks)packages/mobile-sdk-alpha/scripts/shimConfigs.js(1 hunks)packages/mobile-sdk-alpha/tsconfig.cjs.json(1 hunks)packages/mobile-sdk-alpha/tsup.config.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/mobile-sdk-alpha/scripts/postBuild.mjs (1)
packages/mobile-sdk-alpha/scripts/shimConfigs.js (2)
shimConfigs(2-2)shimConfigs(2-2)
🔇 Additional comments (7)
packages/mobile-sdk-alpha/tsconfig.cjs.json (1)
1-10: LGTM! Clean CommonJS configuration.The TypeScript configuration properly extends the base config and sets appropriate CommonJS-specific options. The
esModuleInteropsetting ensures proper interoperability between ESM and CJS modules, which is crucial for the dual-format build pipeline.packages/mobile-sdk-alpha/scripts/shimConfigs.js (1)
2-2: Relative Path Resolution VerifiedThe
targetPath: '../esm/browser.js'correctly includes the/esm/segment, so in the post-build script:
.replace('/esm/', '/cjs/').replace('.js', '.cjs')yields../cjs/browser.cjs.replace('.js', '')yields../esm/browserWhen shims are emitted under
dist/browser/, these relative imports point todist/esm/browser.js(and its CJS equivalent) as intended. No changes needed.packages/mobile-sdk-alpha/scripts/postBuild.mjs (2)
1-11: LGTM! Proper ESM imports and setup.Good use of Node.js built-in modules with the
node:prefix and proper ESMimport.meta.urlhandling for directory resolution.
34-35: String replacement logic is valid for current shimConfigs
AlltargetPathentries inpackages/mobile-sdk-alpha/scripts/shimConfigs.jsuse the/esm/….jspattern, and our test confirms they transform correctly to/cjs/….cjsand strip the.jsfor DTS output. No mismatches were found.packages/mobile-sdk-alpha/tsup.config.ts (1)
3-32: Excellent dual-build configuration!The tsup configuration is well-designed with several smart choices:
Efficient DTS generation: Only the ESM build generates TypeScript declarations (
dts: true), avoiding duplication since both builds would produce identical.d.tsfiles.Proper cleaning strategy:
clean: trueonly on the ESM build prevents race conditions during concurrent builds.Appropriate settings:
splitting: falseis correct for library builds, andtarget: 'es2020'provides good compatibility.Correct tsconfig usage: Each build uses its appropriate TypeScript configuration.
packages/mobile-sdk-alpha/package.json (2)
14-28: Well-structured exports map for dual-package setup!The exports configuration follows Node.js dual-package best practices:
Proper condition ordering:
typesfirst, followed by environment-specific conditions (react-native,browser), then module format conditions (import,require), and finallydefault.Comprehensive browser support: The new
./browserexport provides a clean public API for browser-specific functionality.Consistent path structure: All paths correctly reference the dual build output directories.
36-37: Secure build script with proper cleanup.The build script properly cleans the dist directory before building and includes the post-build step. The use of
rm -rf distis appropriate here since it's a controlled build environment.
Summary
Testing
yarn workspace @selfxyz/mobile-sdk-alpha niceyarn lintyarn buildyarn typesyarn workspace @selfxyz/mobile-sdk-alpha testhttps://chatgpt.com/codex/tasks/task_b_68993d862a14832da2e1822ce0451c6b
Summary by CodeRabbit
New Features
Chores