Removing node-fetch and updating root ca impl for more support
#3782
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller
Unchanged No assets were unchanged |
MikesGlitch
changed the title
Updating root ca impl to use node env variable for more support Removing node-fetch and updating root ca impl to use node env variable for more support
MikesGlitch
changed the title
Removing node-fetch and updating root ca impl to use node env variable for more support Removing node-fetch and updating root ca impl for more support
WalkthroughThe pull request introduces several significant changes across multiple files, primarily focusing on the Possibly related PRs
Suggested labels
Suggested reviewers
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: 1
🧹 Outside diff range and nitpick comments (6)
packages/loot-core/src/platform/server/fetch/index.electron.ts (1)
Line range hint
12-16: Consider enhancing error handling for network-specific issues.The current error handling is basic. Consider adding specific handling for common network-related errors that might occur with fetch.
} catch (error) { - console.error(error); // log error + console.error('Fetch operation failed:', error); + if (error instanceof TypeError) { + // Network errors like DNS failure, network down, etc. + console.error('Network error occurred:', error.message); + } throw error; }packages/loot-core/webpack/webpack.desktop.config.js (1)
31-31: Consider adding a comment explaining the externals configuration.To help future maintainers understand the configuration choices, consider adding a comment explaining why only 'better-sqlite3' is external while fetch functionality uses Electron's native implementation.
- externals: ['better-sqlite3'], + // Only better-sqlite3 is external as we use Electron's native fetch API + // instead of node-fetch for HTTP requests + externals: ['better-sqlite3'],packages/desktop-electron/index.ts (4)
61-76: Consider using asynchronous file reading inloadGlobalPrefsTo prevent blocking the event loop, use
fs.promises.readFileinstead offs.readFileSyncin theloadGlobalPrefsfunction. This change promotes non-blocking I/O operations and improves performance.Suggested change:
-import fs from 'fs'; +import fs from 'fs'; +import { promises as fsPromises } from 'fs'; async function loadGlobalPrefs() { let state: { [key: string]: unknown } | undefined = undefined; try { - state = JSON.parse( - fs.readFileSync( - path.join(process.env.ACTUAL_DATA_DIR!, 'global-store.json'), - 'utf8', - ), - ); + const data = await fsPromises.readFile( + path.join(process.env.ACTUAL_DATA_DIR!, 'global-store.json'), + 'utf8', + ); + state = JSON.parse(data); } catch (e) { console.info('Could not load global state - using defaults'); // This could be the first time running the app - no global-store.json state = {}; } return state; }
81-82: Ensure safe handling of environment variablesWhile spreading
process.envintoenvVariables, verify that no sensitive information (like API keys or secrets) is unintentionally passed to the child process. Consider filtering out unnecessary environment variables.
84-89: Simplify optional chaining in environment variable assignmentSince the presence of
'server-self-signed-cert'is already checked, you can remove the redundant optional chaining in the assignment.Suggested change:
if (globalPrefs?.['server-self-signed-cert']) { envVariables = { ...envVariables, - NODE_EXTRA_CA_CERTS: globalPrefs?.['server-self-signed-cert'], // add self signed cert to env - fetch can pick it up + NODE_EXTRA_CA_CERTS: globalPrefs['server-self-signed-cert'], // add self signed cert to env - fetch can pick it up }; }
97-98: Avoid overwriting existingexecArgvinforkOptionsWhen setting
execArgvinforkOptions, consider appending to any existingexecArgventries to preserve other debugging or profiling flags.Suggested change:
if (isDev) { - forkOptions = { ...forkOptions, execArgv: ['--inspect'] }; + forkOptions = { + ...forkOptions, + execArgv: [...(forkOptions.execArgv || []), '--inspect'] + }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
upcoming-release-notes/3782.mdis excluded by!**/*.mdyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
packages/desktop-electron/index.ts(2 hunks)packages/desktop-electron/package.json(0 hunks)packages/desktop-electron/server.ts(1 hunks)packages/loot-core/package.json(0 hunks)packages/loot-core/src/platform/server/fetch/index.electron.ts(1 hunks)packages/loot-core/src/server/main.ts(0 hunks)packages/loot-core/webpack/webpack.desktop.config.js(1 hunks)
💤 Files with no reviewable changes (3)
- packages/desktop-electron/package.json
- packages/loot-core/package.json
- packages/loot-core/src/server/main.ts
🧰 Additional context used
📓 Learnings (1)
packages/desktop-electron/server.ts (2)
Learnt from: MikesGlitch
PR: actualbudget/actual#3553
File: packages/desktop-electron/server.ts:7-13
Timestamp: 2024-10-04T09:05:56.329Z
Learning: In `packages/desktop-electron/index.ts`, `process.env.lootCoreScript` is correctly assigned a string value based on the `isDev` flag.
Learnt from: MikesGlitch
PR: actualbudget/actual#3553
File: packages/desktop-electron/server.ts:7-13
Timestamp: 2024-10-08T15:46:15.739Z
Learning: In `packages/desktop-electron/index.ts`, `process.env.lootCoreScript` is correctly assigned a string value based on the `isDev` flag.
🔇 Additional comments (7)
packages/loot-core/src/platform/server/fetch/index.electron.ts (2)
Line range hint 8-11: Verify the necessity of the hard-coded origin header.
The addition of origin: 'app://actual' header needs verification. This might be related to Electron's security model or CORS policies.
#!/bin/bash
# Description: Check how the 'app://actual' origin is used in the codebase
# Search for other occurrences of this origin
rg "app://actual"
# Look for security-related code that might validate origins
rg -l "origin.*check|validateOrigin|allowedOrigins"Line range hint 6-11: LGTM! The switch to globalThis.fetch aligns with project goals.
The change successfully removes the node-fetch dependency by utilizing Electron's native Fetch API support. However, since this hasn't been tested on macOS, we should verify the behavior.
✅ Verification successful
The fetch implementation is safe and platform-agnostic
The verification shows:
- The fetch implementation is platform-independent with no platform-specific code paths
- The
app://actualorigin is consistently used across the application:- As the main window's URL in Electron (
desktop-electron/index.ts) - As the origin header in fetch requests (
index.electron.ts)
- As the main window's URL in Electron (
- No platform-specific fetch handling was found that would affect macOS differently
The switch to globalThis.fetch is safe across all platforms as it's a standard Electron feature, not OS-dependent. The origin header is correctly aligned with the application's protocol.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any platform-specific fetch implementations or conditions
# that might affect macOS compatibility
# Look for platform-specific fetch handling
rg -l "process\.platform.*fetch"
# Check for any macOS-specific conditions
rg -l "darwin.*fetch|fetch.*darwin"
Length of output: 329
Script:
#!/bin/bash
# Let's check for any platform-specific fetch implementations and usage patterns
# Search for fetch-related files across the codebase
fd -e ts -e js fetch
# Look for any platform-specific code in fetch-related files
rg -l "process\.platform|darwin|win32|linux" $(fd -e ts -e js fetch)
# Check how the origin header is used in security contexts
rg -C 3 "app://actual" --type ts --type js
# Look for any fetch error handling patterns
ast-grep --pattern 'try {
$$$
fetch($$$)
$$$
} catch'
Length of output: 1499
packages/desktop-electron/server.ts (2)
Line range hint 17-17: Consider making isDev configurable.
The isDev flag is hardcoded to false. Consider making this configurable through environment variables or command-line arguments for better flexibility across different environments.
Let's check if isDev is used consistently across the codebase:
#!/bin/bash
# Description: Check isDev usage patterns
# Expected: Find all locations where isDev is set or used
# Search for isDev assignments and usage
echo "Searching for isDev patterns..."
rg "isDev\s*=|isDev\)" --type ts -A 22-6: LGTM! Good defensive programming practice.
The environment variable check is well-placed and provides a clear error message. This helps catch configuration issues early.
Let's verify that lootCoreScript is always set correctly:
✅ Verification successful
Environment variable check is properly implemented and used
The verification confirms that:
lootCoreScriptis correctly set inpackages/desktop-electron/index.tsbased onisDevflag- The path is properly configured for both development (
loot-core/lib-dist/bundle.desktop.js) and production (resolved path) environments - The variable is used in
lazyLoadBackendfunction to dynamically import and initialize the server bundle - The error handling is appropriate with a descriptive message
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify lootCoreScript environment variable assignment
# Expected: Find all locations where lootCoreScript is set or used
# Search for lootCoreScript assignments and usage
echo "Searching for lootCoreScript assignments and usage..."
rg -l "lootCoreScript" --type ts
# Search specifically for process.env.lootCoreScript assignments
echo -e "\nSearching for process.env.lootCoreScript assignments..."
ast-grep --pattern 'process.env.lootCoreScript = $_'
Length of output: 1951
Script:
#!/bin/bash
# Let's search for lootCoreScript assignments and usage with better context
echo "Searching for lootCoreScript patterns..."
rg "lootCoreScript" --type ts -A 3 -B 3
# Also search for the server bundle initialization
echo -e "\nSearching for server bundle initialization..."
rg "initApp|lazyLoadBackend" --type ts -A 3 -B 3
Length of output: 3284
packages/loot-core/webpack/webpack.desktop.config.js (1)
31-31: LGTM! Verify bundle size impact.
The removal of 'node-fetch' from externals aligns with using Electron's native Fetch API. However, let's verify that this change doesn't significantly impact the bundle size.
✅ Verification successful
Based on the search results, I can now generate the final response:
Removal of node-fetch from externals is correct and safe
The codebase uses a custom fetch implementation from loot-core/src/platform/client/fetch internally, and there are no direct imports or requires of node-fetch. The change to remove it from webpack externals is appropriate since:
- It aligns with using Electron's native Fetch API
- No bundle size concerns as the module wasn't being used anyway
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare bundle size statistics before and after the change
# Note: This assumes stats.json is generated in the current branch
# Get the bundle size statistics
jq '.assets[] | select(.name=="bundle.desktop.js") | .size' stats.json
# Get the main branch stats for comparison
git checkout main
npm run build
jq '.assets[] | select(.name=="bundle.desktop.js") | .size' stats.json
Length of output: 603
Script:
#!/bin/bash
# Let's first check if we can find the webpack config and verify the externals change
rg -U "externals.*\[.*\]" packages/loot-core/webpack/webpack.desktop.config.js -A 2 -B 2
# Let's also check for node-fetch usage in the codebase
rg "require\(['\"]node-fetch['\"]|import.*from.*['\"]node-fetch['\"]"
# Check if there are any fetch-related imports or requires
rg "fetch" --type js -A 2 -B 2
Length of output: 19262
packages/desktop-electron/index.ts (2)
18-19: Confirmed addition of Env and ForkOptions imports
The imported types Env and ForkOptions from 'electron' are necessary for type definitions used later in the code. This enhances type safety and clarity.
78-99: Verify utilityProcess.fork usage and Electron version compatibility
Ensure that the use of utilityProcess.fork with the provided forkOptions is compatible with the Electron version used in the application. The utilityProcess module and its methods are available from Electron v19 onwards.
To confirm the Electron version, you can run the following script:
✅ Verification successful
Let me try a different approach to verify the Electron version by checking the package.json files.
Based on the package.json files, I can now generate the final response since I found that the project uses Electron v30.0.6 in the desktop-electron package's devDependencies.
utilityProcess.fork usage is compatible with current Electron version
The project uses Electron v30.0.6, which is well above the minimum required version (v19) for utilityProcess.fork. The implementation is safe to use with the current Electron version.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the Electron version supports utilityProcess.
# Test: Output Electron's version. Expect: Version >= 19.
electron_version=$(electron -v | cut -d ' ' -f2)
required_version=19.0.0
# Compare versions
if [ "$(printf '%s\n' "$required_version" "$electron_version" | sort -V | head -n1)" = "$required_version" ]; then
echo "Electron version $electron_version is compatible."
else
echo "Electron version $electron_version is not compatible with utilityProcess."
fi
Length of output: 349
Script:
#!/bin/bash
# Search for Electron version in package.json files
fd package.json --exec grep -l "electron" {} \; -x cat {}
Length of output: 5940
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/loot-core/src/platform/server/fetch/index.electron.ts (1)
Line range hint
12-15: Consider enhancing error handling for network-specific issues.The current error handling is basic. Consider adding specific handling for common fetch errors (NetworkError, TimeoutError, etc.) to provide better error messages and potential recovery strategies.
} catch (error) { - console.error(error); // log error + console.error('Fetch operation failed:', { + url: typeof input === 'string' ? input : input.toString(), + error: error instanceof Error ? error.message : String(error) + }); throw error; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
upcoming-release-notes/3782.mdis excluded by!**/*.mdyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
packages/desktop-electron/index.ts(2 hunks)packages/desktop-electron/package.json(0 hunks)packages/desktop-electron/server.ts(1 hunks)packages/loot-core/package.json(0 hunks)packages/loot-core/src/platform/server/fetch/index.electron.ts(1 hunks)packages/loot-core/src/server/main.ts(0 hunks)packages/loot-core/webpack/webpack.desktop.config.js(1 hunks)
💤 Files with no reviewable changes (3)
- packages/desktop-electron/package.json
- packages/loot-core/package.json
- packages/loot-core/src/server/main.ts
🧰 Additional context used
📓 Learnings (1)
packages/desktop-electron/server.ts (2)
Learnt from: MikesGlitch
PR: actualbudget/actual#3553
File: packages/desktop-electron/server.ts:7-13
Timestamp: 2024-10-04T09:05:56.329Z
Learning: In `packages/desktop-electron/index.ts`, `process.env.lootCoreScript` is correctly assigned a string value based on the `isDev` flag.
Learnt from: MikesGlitch
PR: actualbudget/actual#3553
File: packages/desktop-electron/server.ts:7-13
Timestamp: 2024-10-08T15:46:15.739Z
Learning: In `packages/desktop-electron/index.ts`, `process.env.lootCoreScript` is correctly assigned a string value based on the `isDev` flag.
🔇 Additional comments (6)
packages/loot-core/src/platform/server/fetch/index.electron.ts (1)
Line range hint 6-11: LGTM! The switch to native fetch aligns with project goals.
The change successfully removes the node-fetch dependency while maintaining the required functionality. The origin header is properly preserved.
Let's verify the SSL/TLS handling implications:
✅ Verification successful
The switch to native fetch is safe and well-implemented
The verification shows that:
- The SSL/TLS references in
xmlcamt2json.tsare unrelated to fetch implementation (they're just type definitions for CAMT data structures). - The only remaining
node-fetchreference is inpackages/api/index.ts, which provides a fallback implementation whenglobalThis.fetchis not available - this is correct and doesn't affect the Electron implementation.
The change to use native fetch in Electron is safe because:
- Electron's native fetch implementation handles SSL/TLS certificates automatically
- The origin header addition is properly preserved
- The fallback mechanism for non-Electron environments is maintained
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any SSL/TLS configuration that might have been relying on node-fetch
# Look for SSL/TLS related configurations
rg -l "ssl|tls|https.agent|certificateAuthority|ca\." --type ts
# Check for any remaining node-fetch references
rg -l "node-fetch|nodeFetch" --type ts
Length of output: 183
Script:
#!/bin/bash
# Let's check the content of these files to understand the SSL/TLS usage and remaining node-fetch references
# Check xmlcamt2json.ts for SSL/TLS context
rg -A 5 -B 5 "ssl|tls|https.agent|certificateAuthority|ca\." packages/loot-core/src/server/accounts/xmlcamt2json.ts
# Check api/index.ts for node-fetch usage context
cat packages/api/index.ts
Length of output: 3860
packages/desktop-electron/server.ts (2)
2-6: LGTM! Good practice adding environment variable validation.
The early validation of lootCoreScript with a descriptive error message improves the robustness of the application by failing fast when critical configuration is missing.
13-13: Previous comment about error message formatting is still applicable.
The error message formatting issue mentioned in the previous review still needs to be addressed.
packages/loot-core/webpack/webpack.desktop.config.js (2)
31-31: Consider monitoring performance impact.
Since we're now bundling the fetch implementation instead of using it as an external dependency, consider monitoring the application's startup time and memory usage in the Electron environment to ensure there's no significant performance regression.
Let's check if there are any existing performance monitoring implementations:
31-31: LGTM! Verify bundle size impact.
The removal of 'node-fetch' from externals aligns with the PR objective of eliminating this dependency. The change means node-fetch will be bundled instead of being treated as an external dependency.
Let's analyze the bundle size impact:
packages/desktop-electron/index.ts (1)
78-99: Confirm environment variables are correctly propagated to the server process
The additions to createBackgroundProcess correctly set up the environment variables and fork options for the server process. Ensure that all necessary environment variables are included and that the server process utilizes them as intended.
MikesGlitch
deleted the
electron/use-env-ca-auth-instead-of-https-agent
branch
Why
Found while working on this: #3631
Tested on:
Didn't test Mac but should be fine.