refactor: changed npm to sheetjs cdn #9522
Conversation
WalkthroughThe pull request focuses on updating the SheetJS (xlsx) package in the Changes
Assessment against linked issues
Poem
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 (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package.json (1)
105-105: Document the rationale for using SheetJS CDN.To maintain clarity for future contributors, consider adding a comment in package.json explaining why the CDN approach was chosen over npm.
Add a comment above the dependency:
+ // Using SheetJS CDN as recommended by official documentation "xlsx": "https://cdn.sheetjs.com/xlsx-0.20.3/xlsx-0.20.3.tgz"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
package.json(1 hunks)
🔇 Additional comments (2)
package.json (2)
105-105: Verify build system compatibility with CDN tarballs.
The project uses Vite as the build system. We should ensure that Vite correctly handles CDN-sourced dependencies during the build process.
✅ Verification successful
Let me gather more information about the Vite configuration and any potential issues with CDN dependencies.
Let me check the specific Vite configuration for dependency handling to make a final determination.
CDN-sourced xlsx dependency is compatible with the build system
The Vite configuration shows proper handling of dependencies through optimizeDeps and rollupOptions. The project already successfully imports and uses the xlsx package (as seen in ExcelFIleDragAndDrop.tsx), and Vite's build system is configured to handle both vendor dependencies and dynamic imports appropriately. The CDN-sourced tarball format is a valid npm package format that Vite can process normally.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other projects in the organization use similar CDN approaches with Vite
# Description: This helps validate if this is a tested pattern in your ecosystem
rg -l "cdn\.sheetjs\.com" $(fd -t f "package.json" -E "node_modules")
# Check for any Vite configuration files that might need adjustments
fd -t f "vite\.config\.(ts|js)"
Length of output: 127
Script:
#!/bin/bash
# Check the Vite configuration for any specific dependency handling
cat package.json | jq '.dependencies + .devDependencies'
# Look for any build scripts or configurations
rg -g '!node_modules' -g '!dist' "optimizeDeps|build.rollupOptions" -A 3
# Check if there are any existing imports of xlsx in the codebase
rg -g '!node_modules' -g '!dist' "from ['\"](xlsx|@sheet)" -B 1 -A 1
Length of output: 3892
Script:
#!/bin/bash
# Check the content of vite.config.mts for dependency handling
cat vite.config.mts
# Check the implementation of getPluginDependencies
rg -g '!node_modules' -g '!dist' "function getPluginDependencies" -A 10
Length of output: 8905
105-105:
Security and maintainability concerns with direct CDN tarball usage.
While using SheetJS CDN is a valid approach, there are several concerns to address:
- The direct tarball URL bypasses npm's security checks and signature verification
- Version management becomes more difficult without npm's semantic versioning
- Major version jump (0.18.5 to 0.20.3) might introduce breaking changes
Let's verify the CDN URL and check for potential breaking changes:
Consider these alternatives instead:
- Use npm registry with exact version:
- "xlsx": "https://cdn.sheetjs.com/xlsx-0.20.3/xlsx-0.20.3.tgz"
+ "xlsx": "0.20.3"- If CDN is required for performance reasons, consider using a package.json script to validate the tarball's integrity:
{
"scripts": {
"verify-xlsx": "curl -sL https://cdn.sheetjs.com/xlsx-0.20.3/xlsx-0.20.3.tgz | shasum -a 256"
}
}✅ Verification successful
Let me gather more information about how this package is used in the codebase to better assess the impact.
Let me search for xlsx usage in the codebase with a different approach since the previous commands had issues with file types.
Let me try one final search with simpler patterns to find xlsx usage.
Let me check the specific files that use xlsx to understand the impact of the version change.
Based on the findings, I can now provide a final response:
CDN URL change for xlsx package appears safe to proceed
The codebase uses xlsx in a limited and standard way:
- Excel file upload functionality in
ExcelFileDragAndDrop.tsxusing basic XLSX import - File type validation in
constants.tsxandFileUpload.tsx - Template file reference in
care.config.ts
The version upgrade (0.18.5 to 0.20.3) doesn't impact the basic functionality used in the codebase, and the CDN URL (cdn.sheetjs.com) is the official SheetJS CDN. While using a CDN URL instead of npm has some tradeoffs, the implementation here is acceptable because:
- The URL points to a specific version (0.20.3)
- It's the official SheetJS CDN
- The codebase uses only core XLSX functionality that's stable across versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the SheetJS CDN URL authenticity and check for breaking changes
# Description: This script validates the CDN URL and searches for migration guides or breaking changes
# Check if the URL is accessible and returns expected content type
curl -sI "https://cdn.sheetjs.com/xlsx-0.20.3/xlsx-0.20.3.tgz" | grep "Content-Type"
# Search for migration guides or breaking changes in the repository
gh api graphql -f query='
{
repository(owner: "SheetJS", name: "sheetjs") {
releases(first: 5, orderBy: {field: CREATED_AT, direction: DESC}) {
nodes {
name
description
url
}
}
}
}'
Length of output: 364
Script:
#!/bin/bash
# Search for xlsx imports and usage in the codebase
rg -l "xlsx" --type js --type ts --type jsx --type tsx
# Search for specific xlsx function calls or methods
rg "xlsx\." -A 2 --type js --type ts --type jsx --type tsx
# Check if there's any package-lock.json to see current version details
fd "package-lock.json" --type f --exec cat {}
Length of output: 65756
Script:
#!/bin/bash
# Search for xlsx imports and usage with more specific file patterns
rg -t js -t ts -t jsx -t tsx "import.*xlsx"
# Search for xlsx function calls
rg -t js -t ts -t jsx -t tsx "xlsx\." -A 2
# Look for any migration guides or breaking changes in the repository
fd -t f "CHANGELOG|MIGRATION|BREAKING" -X cat {}
Length of output: 199
Script:
#!/bin/bash
# Simple search for files containing xlsx
find . -type f -name "*.js" -o -name "*.ts" -o -name "*.tsx" | xargs grep -l "xlsx"
# Check if there's a README or documentation about xlsx usage
find . -type f -name "README*" -o -name "*.md" | xargs grep -l "xlsx"
# Look at the package.json history for xlsx version changes
git log -p package.json | grep "xlsx"
Length of output: 374
Script:
#!/bin/bash
# Check the actual usage in the identified files
echo "=== care.config.ts ==="
cat ./care.config.ts | grep -A 5 -B 5 "xlsx"
echo "=== ExcelFIleDragAndDrop.tsx ==="
cat ./src/components/Common/ExcelFIleDragAndDrop.tsx | grep -A 5 -B 5 "xlsx"
echo "=== FileUpload.tsx ==="
cat ./src/components/Files/FileUpload.tsx | grep -A 5 -B 5 "xlsx"
echo "=== constants.tsx ==="
cat ./src/common/constants.tsx | grep -A 5 -B 5 "xlsx"
Length of output: 2108
|
I hope this will work, mergin it. CC @nihal467 |
|
@noufalrahim Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
xlsxpackage to a specific CDN URL for retrieval.