fix: adding packages for vite plugins#3688
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThe changes in this pull request involve modifications to several components and configuration files across the project. Specifically, the Changes
Possibly related PRs
Suggested labels
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 (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
mattinannt
left a comment
mattinannt
left a comment
There was a problem hiding this comment.
@pandeymangg Thank you for the fix :-) 🚀
![coderabbitai[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/shareEmbedModal/AppTab.tsx (1)
88-88: LGTM! Consider using a constant for the video path.The change to use a direct string path for the video source is consistent with the PR objectives and simplifies the code. However, for better maintainability and flexibility, consider defining the video path as a constant at the top of the file or in a separate constants file.
Here's a suggestion to improve flexibility:
const CHANGE_SURVEY_TYPE_VIDEO_PATH = "/video/tooltips/change-survey-type-app.mp4"; // Then in the video source: <source src={CHANGE_SURVEY_TYPE_VIDEO_PATH} type="video/mp4" />This approach maintains the simplicity while allowing for easier updates if the path changes in the future.
packages/vite-plugins/copy-compiled-assets/index.ts (3)
1-1: Correct the typo in the commentThere's a typo in the comment on line 1: "plguins" should be "plugins".
39-39: Remove commented-out code for cleanlinessThe commented-out line
// fs.ensureDirSync(outputDir);appears to be leftover code. Removing it will clean up the codebase and avoid confusion.
58-58: Log the actual number of files copiedThe current log message uses
filesToCopy.length, which includes all entries fromdistDir, even those skipped. To reflect the actual number of files copied, count the files during the copy process.Adjust the code as follows:
+ let filesCopied = 0; for (const file of filesToCopy) { // ... await copyFile(srcFile, destFile); + filesCopied++; } console.log( - `Copied ${filesToCopy.length.toString()} files to ${outputDir} (${options.filename})` + `Copied ${filesCopied} files to ${outputDir} (${options.filename})` );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
apps/web/public/video/tooltips/change-survey-type-app.mp4is excluded by!**/*.mp4apps/web/public/video/tooltips/change-survey-type.mp4is excluded by!**/*.mp4pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/shareEmbedModal/AppTab.tsx (1 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/shareEmbedModal/WebsiteTab.tsx (1 hunks)
- packages/copyCompiledAssetsPlugin/vite.config.ts (0 hunks)
- packages/js-core/vite.config.ts (1 hunks)
- packages/surveys/vite.config.ts (1 hunks)
- packages/vite-plugins/.eslintrc.cjs (1 hunks)
- packages/vite-plugins/copy-compiled-assets/index.ts (1 hunks)
- packages/vite-plugins/package.json (1 hunks)
- packages/vite-plugins/tsconfig.json (1 hunks)
💤 Files with no reviewable changes (1)
- packages/copyCompiledAssetsPlugin/vite.config.ts
✅ Files skipped from review due to trivial changes (4)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/shareEmbedModal/WebsiteTab.tsx
- packages/vite-plugins/.eslintrc.cjs
- packages/vite-plugins/package.json
- packages/vite-plugins/tsconfig.json
🧰 Additional context used
🔇 Additional comments (1)
packages/js-core/vite.config.ts (1)
5-5: LGTM! Verify the new import path.The import statement has been correctly updated to reflect the new package structure mentioned in the PR objectives. This change maintains consistency with the new location of Vite plugins.
To ensure the new import path is correct and the plugin is properly implemented, please run the following script:
| import dts from "vite-plugin-dts"; | ||
| import tsconfigPaths from "vite-tsconfig-paths"; | ||
| import { copyCompiledAssetsPlugin } from "../copyCompiledAssetsPlugin/vite.config"; | ||
| import { copyCompiledAssetsPlugin } from "../vite-plugins/copy-compiled-assets"; |
There was a problem hiding this comment.
💡 Codebase verification
Import Path Issue for copyCompiledAssetsPlugin
The new import path points to packages/vite-plugins/copy-compiled-assets.ts, but the file does not exist. Please verify the location of copyCompiledAssetsPlugin or ensure the file is added to packages/vite-plugins.
🔗 Analysis chain
Verify the new import path for copyCompiledAssetsPlugin
The import path for copyCompiledAssetsPlugin has been updated to reflect the new package structure mentioned in the PR objectives. This change aligns with the goal of introducing a new package under packages/vite-plugins for Vite plugins.
However, to ensure this change is correct and doesn't introduce any issues:
- Confirm that the file
copy-compiled-assets.ts(orcopy-compiled-assets/index.ts) exists in thepackages/vite-pluginsdirectory. - Verify that the
copyCompiledAssetsPluginis correctly exported from this new location. - Check if similar changes have been made in other files that might be using this plugin (e.g., other
vite.config.tsfiles in the project).
Run the following script to verify the new structure and usage:
If all checks pass, this change is likely correct and consistent with the PR objectives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new structure and usage of copyCompiledAssetsPlugin
# Test 1: Check if the new file exists
echo "Checking if the new file exists:"
fd --type f --full-path "packages/vite-plugins/copy-compiled-assets.(ts|js|tsx|jsx)"
# Test 2: Verify the export of copyCompiledAssetsPlugin
echo "Verifying the export of copyCompiledAssetsPlugin:"
rg --type typescript "export.*copyCompiledAssetsPlugin" "packages/vite-plugins"
# Test 3: Check for other usages of copyCompiledAssetsPlugin
echo "Checking for other usages of copyCompiledAssetsPlugin:"
rg --type typescript "import.*copyCompiledAssetsPlugin" -g "!packages/surveys/vite.config.ts"
Length of output: 635
Script:
#!/bin/bash
# Description: Verify the new structure and usage of copyCompiledAssetsPlugin
# Test 1: Check if the new file exists
echo "Checking if the new file exists:"
fd --type f --full-path 'packages/vite-plugins/copy-compiled-assets.ts' packages/vite-plugins
# Test 2: Verify the export of copyCompiledAssetsPlugin
echo "Verifying the export of copyCompiledAssetsPlugin:"
rg "export\s+const\s+copyCompiledAssetsPlugin" packages/vite-plugins/copy-compiled-assets.ts
# Test 3: Check for other usages of copyCompiledAssetsPlugin
echo "Checking for other usages of copyCompiledAssetsPlugin:"
rg "import\s+.*copyCompiledAssetsPlugin" --glob '!packages/surveys/vite.config.ts' packages/
Length of output: 785
| try { | ||
| await access(dirPath); | ||
| } catch (error) { | ||
| if ((error as { code: string }).code === "ENOENT") { |
There was a problem hiding this comment.
Use proper error typing for better type safety
In line 15, to safely access the code property, consider casting the error to NodeJS.ErrnoException. This ensures proper typing and better type safety.
Apply this diff to fix the typing:
- if ((error as { code: string }).code === "ENOENT") {
+ if ((error as NodeJS.ErrnoException).code === "ENOENT") {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ((error as { code: string }).code === "ENOENT") { | |
| if ((error as NodeJS.ErrnoException).code === "ENOENT") { |
|
|
||
| for (const file of filesToCopy) { | ||
| const srcFile = path.resolve(distDir, file); | ||
| const destFile = path.resolve(outputDir, file.replace("index", options.filename)); |
There was a problem hiding this comment.
Ensure accurate filename replacement when copying files
The replacement of "index" in filenames on line 48 might unintentionally affect files that have "index" elsewhere in their names. This could lead to incorrect file names after copying.
Consider using a more precise condition:
const destFile = path.resolve(
outputDir,
- file.replace("index", options.filename)
+ file === "index.js" ? `${options.filename}.js` : file
);This change ensures that only files exactly named "index.js" are replaced with the desired filename.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const destFile = path.resolve(outputDir, file.replace("index", options.filename)); | |
| const destFile = path.resolve( | |
| outputDir, | |
| file === "index.js" ? `${options.filename}.js` : file | |
| ); |
| }, | ||
|
|
||
| async writeBundle() { | ||
| const outputDir = path.resolve(config.root, "../../apps/web/public/js"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the output directory configurable
Currently, the outputDir is hardcoded in line 35. Consider making it a configurable option to enhance flexibility and reusability of the plugin.
Modify the CopyCompiledAssetsPluginOptions interface and adjust the code accordingly:
interface CopyCompiledAssetsPluginOptions {
filename: string;
distDir: string;
+ outputDir: string;
}
export function copyCompiledAssetsPlugin(options: CopyCompiledAssetsPluginOptions): Plugin {
let config: ResolvedConfig;
return {
// ...
async writeBundle() {
- const outputDir = path.resolve(config.root, "../../apps/web/public/js");
+ const outputDir = path.resolve(config.root, options.outputDir);
const distDir = path.resolve(config.root, options.distDir);
// ...
},
};
}Committable suggestion was skipped due to low confidence.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
2 participants
What does this PR do?
Adds a new package under
packages/vite-pluginsfor being consistent with our coding conventions regarding the vite plugins as well, right now, the copy compiled assets plugin is defined inside it.Fixes # (issue)
How should this be tested?
pnpm goorpnpm buildapps/web/public/jsdirectoryChecklist
Required
pnpm buildconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
copyCompiledAssetsPluginfor efficient asset management during the build process.Bug Fixes
Chores