remove cross-fetch #490
remove cross-fetch #490
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces significant changes to the project's dependency management and module resolution strategy. The primary modifications involve shifting dependencies to a new Changes
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant PM as Package Management
participant Vendor as @fireproof/vendor
participant Core as @fireproof/core
Dev->>PM: Initiate dependency update
PM->>Vendor: Consolidate dependencies
PM->>Core: Create core package
PM->>Dev: Update import paths
Dev->>Dev: Modify import statements
Dev->>PM: Commit changes
This diagram illustrates the high-level process of consolidating dependencies, creating a core package, and updating import mechanisms across the project. Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (14)
🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
smoke/esm/src/index.test.ts (2)
9-13: Custom “invariant” function introduced.
This is a helpful pattern for clearer runtime checks. Consider employing built-in assertion libraries if desired, but this is acceptable as-is.
14-31: New “action” function logic.
The added steps of verifying document count and updating the label look correct. One minor observation: the temporary document { foo: 1 } appears arbitrary. If meaningful data is relevant for clarity, consider using more descriptive fields.version-copy-package.ts (1)
13-19: New “getVersion” helper function.
Captures the version from GITHUB_REF or uses a default. Consider using optional chaining to simplify the environment check, though the current logic is fine.🧰 Tools
🪛 Biome (1.9.4)
[error] 15-15: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
smoke/esm/it.sh (1)
23-23: Echoing token in logs.
Since this is presumably for testing in a local environment, it’s acceptable. In more sensitive contexts, consider redacting tokens to avoid leaking secrets.
[security]tsconfig.json (1)
Line range hint
32-36: Review module resolution strategy consistencyThere are potential configuration conflicts to address:
- ts-node is configured to use CommonJS while the main compilation uses nodenext
- Paths mapping might need adjustment for the new module resolution strategy
Consider:
- Aligning ts-node's module system with the main configuration
- Updating paths mapping to include
.jsextensions for consistency:"paths": { - "@fireproof/core": ["./src/index.js"], - "use-fireproof": ["./src/react/index.ts"] + "@fireproof/core": ["./src/index.js"], + "use-fireproof": ["./src/react/index.ts"] }Also applies to: 147-152
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (42)
package-fireproof-core.json(1 hunks)package-use-fireproof.json(1 hunks)package.json(3 hunks)smoke/esm/it.sh(2 hunks)smoke/esm/src/index.test.ts(2 hunks)src/blockstore/commitor.ts(2 hunks)src/blockstore/loader-helpers.ts(1 hunks)src/blockstore/loader.ts(1 hunks)src/blockstore/register-store-protocol.ts(1 hunks)src/blockstore/store.ts(1 hunks)src/blockstore/transaction.ts(1 hunks)src/blockstore/types.ts(1 hunks)src/crdt-clock.ts(1 hunks)src/crdt-helpers.ts(2 hunks)src/indexer-helpers.ts(1 hunks)src/react/index.ts(1 hunks)src/react/useAllDocs.ts(1 hunks)src/react/useChanges.ts(1 hunks)src/react/useDocument.ts(1 hunks)src/react/useFireproof.ts(1 hunks)src/react/useLiveQuery.ts(1 hunks)src/runtime/files.ts(1 hunks)src/runtime/gateways/fp-envelope-serialize.ts(1 hunks)src/runtime/keyed-crypto.ts(1 hunks)src/runtime/wait-pr-multiformats/block.ts(1 hunks)src/types.ts(1 hunks)src/utils.ts(1 hunks)tests/blockstore/interceptor-gateway.test.ts(1 hunks)tests/blockstore/keyed-crypto-indexdb-file.test.ts(1 hunks)tests/blockstore/keyed-crypto.test.ts(1 hunks)tests/blockstore/loader.test.ts(1 hunks)tests/blockstore/store.test.ts(1 hunks)tests/blockstore/transaction.test.ts(1 hunks)tests/fireproof/crdt.test.ts(1 hunks)tests/fireproof/hello.test.ts(1 hunks)tests/fireproof/indexer.test.ts(1 hunks)tests/fireproof/utils.test.ts(1 hunks)tests/gateway/file/loader-config.test.ts(1 hunks)tests/gateway/fp-envelope-serialize.test.ts(1 hunks)tests/gateway/indexdb/loader-config.test.ts(1 hunks)tsconfig.json(1 hunks)version-copy-package.ts(3 hunks)
✅ Files skipped from review due to trivial changes (32)
- tests/fireproof/hello.test.ts
- tests/blockstore/transaction.test.ts
- src/crdt-clock.ts
- src/react/useDocument.ts
- src/react/useAllDocs.ts
- src/blockstore/store.ts
- src/react/useLiveQuery.ts
- tests/fireproof/indexer.test.ts
- tests/fireproof/utils.test.ts
- src/runtime/gateways/fp-envelope-serialize.ts
- src/react/useChanges.ts
- src/runtime/wait-pr-multiformats/block.ts
- src/utils.ts
- src/blockstore/transaction.ts
- tests/blockstore/keyed-crypto.test.ts
- src/types.ts
- src/react/index.ts
- tests/blockstore/store.test.ts
- tests/blockstore/interceptor-gateway.test.ts
- src/indexer-helpers.ts
- tests/blockstore/keyed-crypto-indexdb-file.test.ts
- tests/fireproof/crdt.test.ts
- src/react/useFireproof.ts
- src/blockstore/types.ts
- src/blockstore/commitor.ts
- src/blockstore/loader.ts
- src/blockstore/register-store-protocol.ts
- tests/blockstore/loader.test.ts
- tests/gateway/fp-envelope-serialize.test.ts
- src/crdt-helpers.ts
- tests/gateway/file/loader-config.test.ts
- src/runtime/keyed-crypto.ts
🧰 Additional context used
🪛 Biome (1.9.4)
version-copy-package.ts
[error] 15-15: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (17)
tests/gateway/indexdb/loader-config.test.ts (1)
2-2: Consistency with ES module resolution
Specifying the ".js" extension aligns with modern ES module resolution rules. This ensures more explicit and consistent imports across the codebase. Good change!
smoke/esm/src/index.test.ts (3)
4-4: Test name updated to “esm.sh”.
No issues found—this test name change accurately reflects the updated approach.
33-36: Loop-based execution in “main” function.
Running the “action” function multiple times for stress-testing or iterative checks seems valid. No issues identified.
46-46: Final assertion ensures correct label text.
This verifies the last iteration’s output effectively. If you need coverage for earlier iterations, consider additional checks.
src/runtime/files.ts (1)
9-9: Vendor-based import for exporter and ReadableStorage.
Switching to “@fireproof/vendor/ipfs-unixfs-exporter” is consistent with the new vendor transition. Ensure that the registry is accessible and includes the expected versions.
✅ Verification successful
Let me verify the vendor-based import change by checking the codebase.
Based on the search results, I can now verify the vendor-based import change. The results show:
- The
@fireproof/vendorpackage is properly listed as a dependency in the monorepo's package.json with version "^1.0.0" - The import pattern
@fireproof/vendor/ipfs-unixfs-exporteris consistent with other vendor imports in the codebase, as evidenced by multiple similar imports from@fireproof/vendor/across different files - The codebase shows a clear pattern of using vendored dependencies through
@fireproof/vendornamespace for IPFS and IPLD related packages
Vendor import change is properly implemented and consistent with the codebase pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Check if the vendor package exists and is properly referenced
fd "package.json" | xargs cat
# Search for other usages of ipfs-unixfs-exporter
rg "@fireproof/vendor/ipfs-unixfs-exporter"
# Search for any other related imports from this vendor
rg "from \"@fireproof/vendor/" -A 1
Length of output: 17252
version-copy-package.ts (3)
21-22: patchVersion with environment-driven version.
Straightforward assignment of the computed version to packageJson.version. No issues identified.
41-49: transferVersionsFromPackageJson utility function.
Updates destination dependencies with versions from the source in a structured way. Logging an error for missing dependencies helps with debugging. Looks good.
68-74: Combining “@fireproof/core” with main dependencies.
Adding “@fireproof/core” with a caret range and merging it into dependencies and peerDependencies is a solid approach. Verify that all required packages are properly resolved.
✅ Verification successful
Let me verify the package dependencies by examining the package.json files and checking how "@fireproof/core" is being used.
Let me check the package-fireproof-core.json file to verify the dependency configuration.
Based on the review of both the package.json files and the code in question, I can now provide the final response:
The dependency management approach is correctly implemented
The code correctly adds "@fireproof/core" with a caret range to both dependencies and peerDependencies. The package-fireproof-core.json template shows proper dependency structure, and the version-copy-package.ts script ensures version synchronization. The dependencies are properly resolved from the main package.json, and the approach maintains consistency across the monorepo packages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Find and show content of package.json files
echo "=== Main package.json ==="
fd "package.json" --max-depth 1 --exec cat {}
echo -e "\n=== All @fireproof package.json files ==="
fd "package.json" --exec cat {}
Length of output: 15910
Script:
#!/bin/bash
# Find and show content of package-fireproof-core.json
fd "package-fireproof-core.json" --exec cat {}
Length of output: 1773
smoke/esm/it.sh (3)
15-21: Dynamically generated username & user creation.
Appending the timestamp to “admin” is a simple way to generate unique user IDs for testing. However, ensure any sensitive credentials are handled securely.
[security]
39-39: Explicitly adding “@fireproof/core”.
Good practice—this ensures the package is installed at the desired version.
35-35: Copying .npmrc into the working directory.
Ensures the local registry configuration is available. Make sure no private tokens are exposed in this file.
✅ Verification successful
Let me verify the content of the .npmrc file to check for any sensitive information.
Let me try to find any .npmrc files in the repository to understand the context better.
No sensitive information exposed in .npmrc
The .npmrc file only contains a configuration flag enable-pre-post-scripts=true and does not expose any private tokens or sensitive information. The copying operation is safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Check the content of .npmrc in the package directory
cat smoke/esm/package/.npmrc
Length of output: 87
Script:
#!/bin/bash
# Find any .npmrc files in the repository
fd -H .npmrc
# If any files are found, let's see their content
fd -H .npmrc -x cat {}
Length of output: 86
package-use-fireproof.json (1)
32-32: LGTM! Dependency consolidation looks good.
The change to use @fireproof/core aligns with the project's dependency consolidation strategy, moving away from individual dependencies to a more centralized approach.
package-fireproof-core.json (1)
Line range hint 35-45: Verify dependency consolidation strategy
While @fireproof/vendor has been added, there are still several direct dependencies maintained. Consider if these should also be moved under the vendor namespace for consistency:
- multiformats
- @ipld/unixfs
- charwise
- prolly-trees
- idb
- p-limit
- p-map
- p-retry
Let's check if these dependencies are used directly in the codebase or if they could be moved to @fireproof/vendor:
package.json (2)
32-32: LGTM! Keyword addition is appropriate
The addition of the "database" keyword accurately reflects the project's functionality and improves discoverability.
82-82: Verify bundle size impact of new dependencies
The addition of @fireproof/vendor and typedoc should be verified against the bundle size limits defined in bundle-phobia:
- max-size: 390kB
- max-gzip-size: 110kB
- max-overall-size: 1MB
Let's check the impact on bundle size:
Also applies to: 95-95
tsconfig.json (2)
Line range hint 147-147: Ensure type definitions are properly migrated
The TypeRoots configuration has been updated to use "ts-types" instead of "src/missingTypes". Please ensure all custom type definitions have been migrated to the new location.
Let's verify the type definitions migration:
28-28: Verify module resolution compatibility after switching to "nodenext"
The change from "commonjs" to "nodenext" aligns with modern Node.js practices and supports explicit .js extensions, but requires verification of import compatibility across the codebase.
Let's check for potential module resolution issues:
✅ Verification successful
Module resolution changes needed after switching to "nodenext"
Several files in the codebase still use imports without .js extensions, which need to be updated for compatibility with "nodenext". The verification shows:
- Multiple TypeScript files using relative imports without
.jsextensions - Some
package.jsonfiles missing the"type"field
Let's verify the current state of module declarations to ensure a complete assessment:
The "nodenext" module configuration is appropriate and well-supported
The change to "nodenext" in tsconfig.json is correctly aligned with the project's setup:
- Root package.json is properly configured with
"type": "module" - Most sub-packages also use
"type": "module" - The
.jsextensions in imports are already present in core functionality files - Different module configurations in example/test projects are intentional for their specific build environments (bundler, ESNext)
The switch to "nodenext" is a valid enhancement that enforces stricter ESM practices while maintaining compatibility with the existing ESM setup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential module resolution issues after switching to nodenext
# Look for imports without .js extensions that might need updating
rg "from ['\"]\..*['\"]" --type ts
# Check for package.json files that might need "type": "module"
fd package.json --exec jq -r 'select(.type == null) | input_filename'
Length of output: 24749
Script:
#!/bin/bash
# Check package.json type field in the root
jq -r '.type' package.json
# Check for ESM-specific configurations
rg '"type":\s*"module"' -g 'package.json'
# Look for any tsconfig references to module systems
rg '"module":|"moduleResolution":' -g 'tsconfig*.json'
Length of output: 2666
Summary by CodeRabbit
Release Notes
New Features
@fireproof/vendor, across various modules.@fireproof/coreas a new dependency in the project.wait-for-readyin the Docker Compose configuration.Bug Fixes
.jsextensions.Chores
nodenextmodule system.Tests