cli: remove esbuild plugin from rollup config #142
Conversation
jaipaljadeja
force-pushed
the
fix/rollup-build-plugin-esbuild
branch
from
1c43bee to
3421b22
Compare
📝 WalkthroughWalkthroughThis pull request introduces new prerelease JSON metadata for the packages Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as "Test/Caller"
participant VCR as "createVcr()"
participant InternalPlugin as "internalContext Plugin"
participant LoggerPlugin as "Logger Plugin"
Caller->>VCR: Request VCR creation
VCR->>InternalPlugin: Initialize internal context\n(set indexerName & availableIndexers)
VCR->>LoggerPlugin: Append logger plugin
VCR-->>Caller: Return configured VCR with plugins
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
examples/starknet-client/package.json (1)
22-23: Consider pinning the TypeScript version.The addition of TypeScript as a devDependency is good for type safety. However, consider pinning the TypeScript version to avoid potential breaking changes from minor version updates.
Apply this diff to pin the TypeScript version:
- "typescript": "^5.6.2" + "typescript": "5.6.2"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
change/@apibara-indexer-7d0fe9aa-5dc3-4ba2-96c2-26a750edd3ec.json(1 hunks)change/apibara-613baa45-cb22-4333-9430-93f356634a9a.json(1 hunks)examples/cli/test/ethereum.test.ts(1 hunks)examples/cli/test/starknet.test.ts(1 hunks)examples/starknet-client/package.json(1 hunks)packages/cli/src/rollup/config.ts(1 hunks)packages/indexer/src/plugins/context.ts(1 hunks)packages/indexer/src/testing/index.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- change/@apibara-indexer-7d0fe9aa-5dc3-4ba2-96c2-26a750edd3ec.json
- change/apibara-613baa45-cb22-4333-9430-93f356634a9a.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (7)
packages/indexer/src/plugins/context.ts (1)
33-37: LGTM! Improved error handling.The changes enhance error handling by:
- Using an explicit
undefinedcheck instead of a truthy check, preventing false negatives- Providing a more descriptive error message that helps users identify the potential cause
packages/indexer/src/testing/index.ts (1)
32-39: LGTM! Properly configured internalContext plugin.The changes ensure that the internalContext plugin is:
- Always included as the first plugin
- Properly configured with meaningful context values derived from cassetteName
examples/cli/test/ethereum.test.ts (1)
35-35: LGTM! Improved test snapshot.The changes improve test maintainability by excluding implementation details (_id field) from the snapshot, focusing on relevant business data.
packages/cli/src/rollup/config.ts (2)
54-55: LGTM! Added filters for PURE annotation warnings.The changes properly handle PURE annotation warnings by filtering them out.
1-87:❓ Verification inconclusive
Verify the impact of removing esbuild plugin.
The removal of the esbuild plugin might affect:
- ES features transpilation
- Source map generation
- Build performance
Let's check the package.json and tsconfig.json files to ensure proper configuration without esbuild:
🏁 Script executed:
#!/bin/bash # Description: Check TypeScript and build configuration # Check package.json for relevant build dependencies and scripts echo "Checking package.json..." fd -t f "package.json" -x cat {} \; | jq -r '.dependencies,.devDependencies,.scripts' # Check tsconfig.json for target configuration echo "Checking tsconfig.json..." fd -t f "tsconfig.json" -x cat {} \;Length of output: 12309
Verify Removal of esbuild Plugin Impact
Based on our review of the build configuration:
- Transpilation: TypeScript settings and the current Rollup plugins (e.g. esmShim, commonjs, nodeResolve) appear to adequately handle ES feature transpilation.
- Source Maps: Sourcemap generation is explicitly enabled in the Rollup output configuration, so this aspect remains intact.
- Build Performance: Removing the esbuild plugin might affect build speed if it was previously accelerating transpilation. Verify that the alternative process meets performance expectations.
If the removal of the esbuild plugin is intentional, consider cleaning up its related dependencies from package.json to avoid confusion.
examples/cli/test/starknet.test.ts (2)
35-35: LGTM! Simplified snapshot assertion.The change to exclude
_idfrom the snapshot assertion helps focus on the essential transaction details (hash and number).
35-35: Verify if_cursorshould also be excluded from the snapshot.The AI summary indicates that both
_idand_cursorproperties should be excluded, but the code only excludes_id. Please verify if_cursorshould also be excluded from the snapshot assertion.Likely an incorrect or invalid review comment.
2 participants
removes
esbuildplugin from rollup config due to some issues with build, also adds missinginternalContextplugin fromvcrand updates tests forcliexample.