feat(custom-routing): Refactor internally to use new template #182
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Caution Review failedThe pull request is closed. WalkthroughThe changes involve the removal of several files related to the custom routing package, including ESLint configuration, Changes
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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: 2
🧹 Outside diff range and nitpick comments (6)
packages/custom-routing/tsup.config.ts (2)
4-9: Add error handling for package.json parsingThe current implementation might throw uncaught exceptions if the package.json file is missing or malformed. Consider adding proper error handling and type safety.
-const packageJson = JSON.parse(readFileSync('./package.json', 'utf-8')); +interface PackageJson { + dependencies?: Record<string, string>; + peerDependencies?: Record<string, string>; + devDependencies?: Record<string, string>; +} + +let packageJson: PackageJson; +try { + packageJson = JSON.parse(readFileSync('./package.json', 'utf-8')); +} catch (error) { + console.error('Failed to parse package.json:', error); + packageJson = {}; +}
11-28: Configuration looks good with some suggestionsThe bundler configuration is well-structured for a library package. A few suggestions for consideration:
- The
formatcould include 'cjs' if CommonJS support is needed- Consider enabling
shims: truefor better compatibility if browser usage is intended- The chunk naming strategy might generate long filenames
export default defineConfig({ entry: ['src/index.ts'], - format: ['esm'], + format: ['esm', 'cjs'], target: 'node18', bundle: true, dts: true, sourcemap: true, clean: true, splitting: true, minify: false, external: dependencies, noExternal: devDependencies, treeshake: 'smallest', tsconfig: 'tsconfig.json', + shims: true, esbuildOptions: (options) => { - options.chunkNames = 'chunks/[name]-[hash]'; + options.chunkNames = '[name]-[hash]'; }, });packages/custom-routing/src/index.ts (2)
5-6: Offer to assist with adding support for inline API routesThe TODO comment indicates a pending feature to support inline API routes once Astro core accepts virtual modules as injected routes. I can help implement this feature or open a GitHub issue to track it if you'd like.
33-34: Fix grammatical error in the error messageThe error message contains a grammatical error. Please change
'Either use disable strict mode of custom routing or remove any file-based routes.'to'Either disable strict mode of custom routing or remove any file-based routes.'Apply this diff to correct the error message:
- 'Either use disable strict mode of custom routing or remove any file-based routes.' + 'Either disable strict mode of custom routing or remove any file-based routes.'packages/custom-routing/package.json (2)
28-28: UseprepublishOnlyinstead ofprepublishIt's recommended to use
prepublishOnlyinstead ofprepublishto avoid unexpected script executions during package installation.Apply this diff:
- "prepublish": "pnpm run build", + "prepublishOnly": "pnpm run build",
23-23: Consider excludingsrcfrom the package distributionIncluding the
srcdirectory in thefilesarray will include source files in the published package, potentially increasing package size. If this is unintended, consider removingsrcfromfiles.Apply this diff to exclude
srcfrom the package files:"files": [ "README.md", "dist", - "src" ],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
packages/custom-routing/.eslintrc.cjs(0 hunks)packages/custom-routing/gitignore(0 hunks)packages/custom-routing/index.ts(0 hunks)packages/custom-routing/npmignore(1 hunks)packages/custom-routing/package.json(1 hunks)packages/custom-routing/src/index.ts(1 hunks)packages/custom-routing/tsup.config.ts(1 hunks)
💤 Files with no reviewable changes (3)
- packages/custom-routing/gitignore
- packages/custom-routing/.eslintrc.cjs
- packages/custom-routing/index.ts
✅ Files skipped from review due to trivial changes (1)
- packages/custom-routing/npmignore
🔇 Additional comments (5)
packages/custom-routing/tsup.config.ts (1)
14-14: Verify Node.js version compatibility
The configuration targets Node.js 18. Please ensure this aligns with the project's minimum supported Node.js version.
✅ Verification successful
Let me try a different approach to check the Node.js requirements.
Let me try one more approach to check the Node.js requirements by looking at the package manager configuration and dependencies.
Node.js 18 target is appropriate
The target of node18 in the tsup configuration is appropriate based on the project's dependencies and requirements:
- The project uses modern dependencies like Vite 6, TypeScript 5.7, and Astro 5
@types/nodeis at version 22.x in the catalog- The build tooling (tsup 8.3.5) and test framework (Vitest 2.x) all support Node.js 18
These dependencies indicate the project is using current Node.js features, making Node.js 18 a suitable minimum target that balances modern features with broad compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check Node.js version requirements across the project
# Check for Node.js version specifications in package.json files
echo "Checking Node.js version requirements in package.json files:"
fd package.json --exec jq -r '.engines.node // empty'
# Check for .nvmrc or .node-version files
echo -e "\nChecking for Node.js version files:"
fd -e nvmrc -e node-version --exec cat
Length of output: 428
Script:
#!/bin/bash
# Description: Check Node.js version requirements with a more robust approach
# Check root package.json
echo "Root package.json engines field:"
cat package.json | grep -A 5 '"engines"' || true
# Check custom-routing package.json
echo -e "\nCustom routing package.json engines field:"
cat packages/custom-routing/package.json | grep -A 5 '"engines"' || true
# Look for any Node version references in README
echo -e "\nNode.js version mentions in README:"
rg -i "node(\s+)?(\d+)" README.md || true
# Check for any other version requirements in workspace files
echo -e "\nChecking other workspace package.json files:"
find . -name "package.json" -type f -exec sh -c 'echo "=== $1 ==="; cat "$1" | grep -A 5 "engines"' sh {} \;
Length of output: 2509
Script:
#!/bin/bash
# Check for Node.js version requirements in package manager files
echo "Checking pnpm-workspace.yaml:"
cat pnpm-workspace.yaml || true
echo -e "\nChecking .npmrc:"
cat .npmrc || true
echo -e "\nChecking dependencies in custom-routing package.json:"
cat packages/custom-routing/package.json | jq '.dependencies, .devDependencies, .peerDependencies' || true
echo -e "\nChecking dependencies in root package.json:"
cat package.json | jq '.dependencies, .devDependencies' || true
Length of output: 2040
packages/custom-routing/src/index.ts (3)
14-42: customRoutingInner function is well-implemented
The function correctly constructs the AstroIntegration object with appropriate hooks, including conditional logic based on the strict option.
44-49: customRouting function correctly initializes the integration
The function correctly initializes customRoutingInner with strict: false.
51-56: strictCustomRouting function correctly initializes the integration in strict mode
The function correctly initializes customRoutingInner with strict: true.
packages/custom-routing/package.json (1)
15-18: exports field is correctly configured
The exports field is appropriately set to provide type definitions and the default export.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Luiz Ferraz <[email protected]>
Summary by CodeRabbit
New Features
tsupbundler to streamline the build process.Bug Fixes
.gitignoreconfigurations, allowing default behaviors to take effect.Documentation
package.jsonto reflect new scripts and dependency management.Chores
.npmignoreto include thesrcdirectory for npm packaging.