Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use rollup #4928

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

use rollup #4928

wants to merge 2 commits into from

Conversation

vio
Copy link
Member

@vio vio commented Jan 12, 2025

  • *build: Eslint - accept devDependencies on .config.js/mjs/ts
  • build(rollup-plugin): Bundle with rollup

Summary by CodeRabbit

Release Notes

  • Build Configuration

    • Updated project build process to use Rollup for module bundling.
    • Configured TypeScript compilation for Node.js 16 compatibility.
  • Package Management

    • Restructured package output directories.
    • Updated entry points and export configurations.
    • Added Rollup TypeScript plugin for build process.
  • Development

    • Modified ESLint configuration to allow configuration file dependencies.
    • Updated .gitignore to track library directories and exclude the distribution folder.

Copy link

coderabbitai bot commented Jan 12, 2025

Walkthrough

This pull request focuses on enhancing the configuration and build process for the Rollup plugin package. Changes include updates to the ESLint rules for dependency management, modifications to the .gitignore file, and adjustments in the package.json to support new module formats. Additionally, new Rollup and TypeScript configurations have been introduced, which streamline the build output and improve compatibility with Node.js version 16.

Changes

File Change Summary
.eslintrc.js Added configuration file patterns **/*.config.{js,mjs} and **/*.config.ts to import/no-extraneous-dependencies rule
packages/rollup-plugin/.gitignore Removed lib, lib-esm, types from ignore list; added dist to ignore list
packages/rollup-plugin/package.json - Updated main entry to ./dist/index.cjs
- Added module entry ./dist/index.mjs
- Introduced exports field
- Modified build and clean scripts
- Added @rollup/plugin-typescript dev dependency
packages/rollup-plugin/rollup.config.mjs Added new Rollup configuration with ESM and CommonJS output configurations
packages/rollup-plugin/tsconfig.json - Added module: "Node16"
- Updated declarationDir to ./dist

Sequence Diagram

sequenceDiagram
    participant Dev as Developer
    participant Build as Rollup Build
    participant TS as TypeScript Compiler
    participant Dist as Dist Directory

    Dev->>Build: Trigger build
    Build->>TS: Compile source
    TS-->>Build: Generate compiled files
    Build->>Dist: Output ESM module
    Build->>Dist: Output CommonJS module
    Build->>Dist: Generate type declarations
Loading

Possibly related PRs

  • Error with v418 vite maximum call stack size exceeded rltv 1169 #4951: The changes in packages/rollup-plugin/package.json regarding the update of the rollup-plugin-webpack-stats dependency may relate to the overall configuration and dependency management improvements in the main PR's .eslintrc.js, as both involve managing dependencies within the Rollup plugin ecosystem.

Poem

🐰 Rollup's dance, a config delight,
Modules now shine, oh so bright!
From lib to dist, we've made our mark,
TypeScript singing, a build system's spark!
Hop, hop, hooray for clean code's might! 🚀


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3920908 and b6ecf9b.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (5)
  • .eslintrc.js (2 hunks)
  • packages/rollup-plugin/.gitignore (1 hunks)
  • packages/rollup-plugin/package.json (3 hunks)
  • packages/rollup-plugin/rollup.config.mjs (1 hunks)
  • packages/rollup-plugin/tsconfig.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
  • packages/rollup-plugin/.gitignore
  • packages/rollup-plugin/rollup.config.mjs
  • .eslintrc.js
  • packages/rollup-plugin/tsconfig.json
  • packages/rollup-plugin/package.json
⏰ Context from checks skipped due to timeout of 90000ms (16)
  • GitHub Check: webpack5 plugin ubuntu-latest nodejs 14
  • GitHub Check: webpack4 plugin ubuntu-latest nodejs 14
  • GitHub Check: webpack5 plugin ubuntu-latest nodejs 16
  • GitHub Check: webpack4 plugin ubuntu-latest nodejs 16
  • GitHub Check: rollup 4 plugin nodejs 18
  • GitHub Check: webpack5 plugin ubuntu-latest nodejs 18
  • GitHub Check: CLI ubuntu-latest nodejs 14
  • GitHub Check: rollup 3 plugin nodejs 16
  • GitHub Check: webpack5 plugin ubuntu-latest nodejs 20
  • GitHub Check: CLI ubuntu-latest nodejs 16
  • GitHub Check: webpack5 plugin windows-latest nodejs 14
  • GitHub Check: CLI ubuntu-latest nodejs 18
  • GitHub Check: test-unit
  • GitHub Check: webpack4 plugin windows-latest nodejs 14
  • GitHub Check: CLI ubuntu-latest nodejs 20
  • GitHub Check: test-e2e

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

socket-security bot commented Jan 12, 2025

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@rollup/[email protected] environment, filesystem +3 308 kB shellscape
npm/[email protected] None +2 124 kB iamturns
npm/[email protected] None +2 180 kB ljharb
npm/[email protected] None 0 59.4 kB jounqin, lydell, thorn0
npm/[email protected] Transitive: environment +1 58.5 kB ljharb
npm/[email protected] environment, filesystem Transitive: eval, shell, unsafe +23 6.12 MB eslintbot

View full report↗︎

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
packages/rollup-plugin/rollup.config.mjs (1)

1-27: LGTM! Well-structured dual-format build configuration.

The configuration correctly handles both ESM and CommonJS outputs with proper file extensions and sourcemaps.

Consider adding production optimizations:

 export default defineConfig([
   {
     input: './src/index.ts',
     output: {
       dir: './dist',
       format: 'esm',
       entryFileNames: '[name].mjs',
       chunkFileNames: 'chunks/[name].mjs',
       sourcemap: true,
+      generatedCode: 'es2015',
+      minifyInternalExports: true
     },
     plugins: [typescript({ tsconfig: './tsconfig.json' })],
+    external: ['rollup'], // Assuming rollup is a peerDependency
   },
   // Similar changes for CommonJS config
 ]);
packages/rollup-plugin/tsconfig.json (1)

7-7: Consider additional TypeScript options for better build output.

While moving declarations to './dist' is correct, consider adding these options for improved output:

 {
   "compilerOptions": {
     "noEmit": false,
     "module": "Node16",
     "declaration": true,
     "declarationDir": "./dist",
+    "declarationMap": true,
+    "sourceMap": true,
+    "stripInternal": true
   }
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a72863b and 5071d31.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (5)
  • .eslintrc.js (2 hunks)
  • packages/rollup-plugin/.gitignore (1 hunks)
  • packages/rollup-plugin/package.json (4 hunks)
  • packages/rollup-plugin/rollup.config.mjs (1 hunks)
  • packages/rollup-plugin/tsconfig.json (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: build
🔇 Additional comments (6)
.eslintrc.js (1)

46-46: LGTM! Appropriate ESLint rules for config files.

The added patterns correctly allow configuration files to be imported as devDependencies, which is necessary for the Rollup setup.

Also applies to: 87-87

packages/rollup-plugin/.gitignore (1)

10-10: LGTM! Updated ignore patterns match new build structure.

The addition of 'dist' to .gitignore aligns with the new Rollup output configuration.

packages/rollup-plugin/tsconfig.json (1)

5-5: LGTM! Appropriate module resolution for Node.js compatibility.

The "Node16" module setting ensures proper resolution for both ESM and CommonJS formats.

packages/rollup-plugin/package.json (3)

Line range hint 60-69: LGTM! Dependencies are well configured

The dependency changes look good:

  • Added TypeScript plugin for Rollup
  • Appropriate version ranges in both dev and peer dependencies
  • Consistent with the migration to Rollup

Line range hint 1-76: Verify package.json completeness

The package.json appears complete but let's verify all essential fields are present and correctly configured for a public npm package.

Run these checks:

✅ Verification successful

Package.json configuration is complete and correct

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for essential package.json fields
jq -r '. | select(.name and .version and .main and .license and .repository)' packages/rollup-plugin/package.json

# Verify package name availability
npm view rollup-plugin-bundle-stats

# Check if any similar packages exist
npm search rollup-plugin-bundle

Length of output: 8741


23-24: Verify TypeScript type generation in the build process

The build script might need to explicitly generate TypeScript declaration files. The current Rollup configuration should be verified to ensure it handles type generation correctly.

Run this script to check if the Rollup config includes TypeScript type generation:

✅ Verification successful

Type generation is properly configured

The build process already includes TypeScript type generation through @rollup/plugin-typescript with proper configuration in both ESM and CommonJS build targets.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if rollup config handles type generation
ast-grep --pattern 'plugin($typescript|$ts)($$$)' packages/rollup-plugin/rollup.config.mjs

Length of output: 92


Script:

#!/bin/bash
# Check rollup config content
echo "=== Rollup Config ==="
cat packages/rollup-plugin/rollup.config.mjs

echo -e "\n=== TypeScript Config ==="
fd tsconfig.json packages/rollup-plugin/

echo -e "\n=== Package Dependencies ==="
cat packages/rollup-plugin/package.json | jq '.dependencies, .devDependencies'

Length of output: 1462

packages/rollup-plugin/package.json Outdated Show resolved Hide resolved
Copy link

relativeci bot commented Jan 12, 2025

#11867 Bundle Size — 380.78KiB (0%).

b6ecf9b(current) vs 15ce872 master#11866(baseline)

Warning

Bundle contains 2 duplicate packages – View duplicate packages

Bundle metrics  no changes
                 Current
#11867
     Baseline
#11866
No change  Initial JS 333.89KiB 333.89KiB
No change  Initial CSS 46.89KiB 46.89KiB
No change  Cache Invalidation 0% 0%
No change  Chunks 3 3
No change  Assets 4 4
No change  Modules 700 700
No change  Duplicate Modules 0 0
No change  Duplicate Code 0% 0%
No change  Packages 39 39
No change  Duplicate Packages 1 1
Bundle size by type  no changes
                 Current
#11867
     Baseline
#11866
No change  JS 333.89KiB 333.89KiB
No change  CSS 46.89KiB 46.89KiB

Bundle analysis reportBranch use-rollupProject dashboard


Generated by RelativeCIDocumentationReport issue

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🔭 Outside diff range comments (1)
packages/rollup-plugin/package.json (1)

Line range hint 48-62: Review dependencies configuration

  1. Consider using caret ranges for Rollup plugins to ease updates
  2. Some Rollup plugins are outdated and should be updated
  3. rollup-plugin-webpack-stats should be in dependencies as it's likely needed at runtime

Apply these changes:

-    "@rollup/plugin-commonjs": "28.0.2",
-    "@rollup/plugin-json": "6.1.0",
-    "@rollup/plugin-node-resolve": "16.0.0",
-    "@rollup/plugin-typescript": "12.1.2",
+    "@rollup/plugin-commonjs": "^25.0.0",
+    "@rollup/plugin-json": "^6.1.0",
+    "@rollup/plugin-node-resolve": "^25.0.0",
+    "@rollup/plugin-typescript": "^12.1.2",

Move webpack-stats plugin back to dependencies:

-  "devDependencies": {
-    "rollup-plugin-webpack-stats": "0.4.1",
+  "dependencies": {
+    "rollup-plugin-webpack-stats": "^0.4.1",
🧹 Nitpick comments (1)
packages/rollup-plugin/package.json (1)

15-16: Improve build scripts configuration

  1. The build script changes directory which can be error-prone
  2. The clean script should also remove the types directory
  3. Missing script to generate type definitions

Consider this improved configuration:

-    "build": "npm run clean && cd ../../ && rollup -c packages/rollup-plugin/rollup.config.mjs",
-    "clean": "rimraf dist",
+    "build": "npm run clean && rollup -c rollup.config.mjs",
+    "build:types": "tsc --emitDeclarationOnly --outDir dist",
+    "build:all": "npm run build && npm run build:types",
+    "clean": "rimraf dist types",
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5071d31 and 90e2063.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (5)
  • packages/rollup-plugin/.eslintignore (1 hunks)
  • packages/rollup-plugin/.gitignore (1 hunks)
  • packages/rollup-plugin/package.json (4 hunks)
  • packages/rollup-plugin/rollup.config.mjs (1 hunks)
  • packages/rollup-plugin/tsconfig.json (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • packages/rollup-plugin/.eslintignore
🚧 Files skipped from review as they are similar to previous changes (3)
  • packages/rollup-plugin/rollup.config.mjs
  • packages/rollup-plugin/.gitignore
  • packages/rollup-plugin/tsconfig.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: build
  • GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/rollup-plugin/package.json (1)

5-7: 🛠️ Refactor suggestion

Add module field and align output paths

  1. Add the module field for ESM support
  2. Ensure consistency between output paths:
    • main points to dist/
    • types points to types/
    • Consider moving type definitions to dist/ for consistency

Apply this diff:

   "main": "./dist/index.js",
+  "module": "./dist/index.mjs",
   "types": "types/index.d.ts",

Also, consider adding an exports field to support both CJS and ESM:

+  "exports": {
+    ".": {
+      "types": "./dist/index.d.ts",
+      "import": "./dist/index.mjs",
+      "require": "./dist/index.js"
+    }
+  },

Likely invalid or redundant comment.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
packages/html-templates/scripts/bundle-template.js (1)

12-13: Consider using async file operations.

For better performance with large files, consider using async versions of the file operations.

-fs.mkdirSync(OUT_DIR, { recursive: true });
-fs.writeFileSync(path.join(OUT_DIR, 'index.js'), output);
+async function writeOutput(output) {
+  await fs.promises.mkdir(OUT_DIR, { recursive: true });
+  await fs.promises.writeFile(path.join(OUT_DIR, 'index.js'), output);
+}
+writeOutput(output).catch(error => {
+  console.error('Failed to write output:', error);
+  process.exit(1);
+});
packages/html-templates/package.json (1)

9-10: Consider adding build script validation.

The new two-step build process could benefit from additional validation to ensure the template build succeeds before running the bundler.

-    "build:template": "cross-env NODE_ENV=production webpack --config webpack.config.production.js --mode production",
-    "build": "npm run build:template && ./scripts/bundle-template.js",
+    "build:template": "cross-env NODE_ENV=production webpack --config webpack.config.production.js --mode production",
+    "validate:template": "node -e \"require('fs').accessSync('./dist-template/index.html')\"",
+    "build": "npm run build:template && npm run validate:template && ./scripts/bundle-template.js",
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 90e2063 and 1bd40d5.

📒 Files selected for processing (5)
  • packages/html-templates/.gitignore (1 hunks)
  • packages/html-templates/index.js (0 hunks)
  • packages/html-templates/package.json (1 hunks)
  • packages/html-templates/scripts/bundle-template.js (1 hunks)
  • packages/html-templates/webpack.config.common.js (1 hunks)
💤 Files with no reviewable changes (1)
  • packages/html-templates/index.js
✅ Files skipped from review due to trivial changes (1)
  • packages/html-templates/.gitignore
🔇 Additional comments (2)
packages/html-templates/webpack.config.common.js (1)

12-12: Verify architectural decision regarding static dist directory.

The change from a configurable distDir to a static path might reduce flexibility. Please confirm if this is intentional as part of the migration to Rollup.

Also applies to: 18-19

✅ Verification successful

Static dist-template directory is aligned with the codebase patterns

The change to a static dist-template directory appears intentional and well-structured, following the pattern of dedicated output directories across the codebase. This specific naming suggests a purposeful separation of template outputs, which aligns with the Rollup migration strategy.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if other packages use similar static dist directory pattern
rg -g '*.js' -g '*.ts' "path\.join\(__dirname,\s*['\"]dist" --type-add 'js:*.{js,ts,jsx,tsx}'

Length of output: 1808

packages/html-templates/package.json (1)

5-5: LGTM! Main entry point update aligns with new build output.

The update to use dist/index.js as the main entry point correctly reflects the new build structure.

Comment on lines 9 to 10
const template = fs.readFileSync(path.join(ROOT_DIR, 'dist-template', 'index.html'), 'utf8');
const output = `module.exports = decodeURI("${encodeURI(template)}");`;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling for file operations.

The synchronous file operations could fail if the file doesn't exist or has incorrect permissions. Consider adding try-catch blocks and validation.

-const template = fs.readFileSync(path.join(ROOT_DIR, 'dist-template', 'index.html'), 'utf8');
-const output = `module.exports = decodeURI("${encodeURI(template)}");`;
+try {
+  const templatePath = path.join(ROOT_DIR, 'dist-template', 'index.html');
+  if (!fs.existsSync(templatePath)) {
+    throw new Error('Template file not found');
+  }
+  const template = fs.readFileSync(templatePath, 'utf8');
+  const output = `module.exports = decodeURI("${encodeURI(template)}");`;
+} catch (error) {
+  console.error('Failed to process template:', error);
+  process.exit(1);
+}
📝 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.

Suggested change
const template = fs.readFileSync(path.join(ROOT_DIR, 'dist-template', 'index.html'), 'utf8');
const output = `module.exports = decodeURI("${encodeURI(template)}");`;
try {
const templatePath = path.join(ROOT_DIR, 'dist-template', 'index.html');
if (!fs.existsSync(templatePath)) {
throw new Error('Template file not found');
}
const template = fs.readFileSync(templatePath, 'utf8');
const output = `module.exports = decodeURI("${encodeURI(template)}");`;
} catch (error) {
console.error('Failed to process template:', error);
process.exit(1);
}

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
packages/rollup-plugin/package.json (1)

15-15: Improve build script robustness

The build script uses cd to navigate directories which can be fragile. Consider using workspace-relative paths instead.

-    "build": "npm run clean && cd ../../ && rollup -c packages/rollup-plugin/rollup.config.mjs",
+    "build": "npm run clean && rollup -c rollup.config.mjs",
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1bd40d5 and fee6577.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (11)
  • .eslintrc.js (2 hunks)
  • packages/html-templates/.gitignore (1 hunks)
  • packages/html-templates/index.js (0 hunks)
  • packages/html-templates/package.json (1 hunks)
  • packages/html-templates/scripts/bundle-template.js (1 hunks)
  • packages/html-templates/webpack.config.common.js (1 hunks)
  • packages/rollup-plugin/.eslintignore (1 hunks)
  • packages/rollup-plugin/.gitignore (1 hunks)
  • packages/rollup-plugin/package.json (4 hunks)
  • packages/rollup-plugin/rollup.config.mjs (1 hunks)
  • packages/rollup-plugin/tsconfig.json (1 hunks)
💤 Files with no reviewable changes (1)
  • packages/html-templates/index.js
🚧 Files skipped from review as they are similar to previous changes (9)
  • packages/html-templates/.gitignore
  • packages/rollup-plugin/.eslintignore
  • packages/rollup-plugin/.gitignore
  • .eslintrc.js
  • packages/rollup-plugin/rollup.config.mjs
  • packages/rollup-plugin/tsconfig.json
  • packages/html-templates/webpack.config.common.js
  • packages/html-templates/scripts/bundle-template.js
  • packages/html-templates/package.json
⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: CodeQL
  • GitHub Check: build
  • GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
packages/rollup-plugin/package.json (2)

5-6: ⚠️ Potential issue

Fix inconsistent output directory paths

There's an inconsistency between the output paths:

  • main points to dist/ directory
  • types points to types/ directory

This could cause module resolution issues.

Apply this diff to align the paths:

  "main": "./dist/index.js",
-  "types": "types/index.d.ts",
+  "types": "./dist/index.d.ts",

Likely invalid or redundant comment.


49-52: Consider using caret ranges for Rollup plugins

Using fixed versions for Rollup plugins can make updates harder. Consider using caret ranges to allow minor version updates.

Let's verify if newer versions are available and check for any security advisories:

Apply this diff to use caret ranges:

-    "@rollup/plugin-commonjs": "28.0.2",
-    "@rollup/plugin-json": "6.1.0",
-    "@rollup/plugin-node-resolve": "16.0.0",
-    "@rollup/plugin-typescript": "12.1.2",
+    "@rollup/plugin-commonjs": "^28.0.2",
+    "@rollup/plugin-json": "^6.1.0",
+    "@rollup/plugin-node-resolve": "^16.0.0",
+    "@rollup/plugin-typescript": "^12.1.2",

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
packages/rollup-plugin/package.json (2)

Line range hint 19-21: Remove outdated directories configuration.

The directories field still references the old lib directory which is no longer used.

Apply this diff to remove the outdated configuration:

-  "directories": {
-    "lib": "lib"
-  },

8-14: Enhance package exports configuration.

The exports field could be improved by:

  1. Adding a default export condition
  2. Ensuring consistent path format

Apply this diff to enhance the exports configuration:

   "exports": {
     ".": {
+      "default": "./dist/index.mjs",
       "import": "./dist/index.mjs",
       "require": "./dist/index.cjs",
-      "types": "./dist/index.d.ts"
+      "types": "./types/index.d.ts"
     }
   },
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fee6577 and 3920908.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (5)
  • .eslintrc.js (2 hunks)
  • packages/rollup-plugin/.gitignore (1 hunks)
  • packages/rollup-plugin/package.json (3 hunks)
  • packages/rollup-plugin/rollup.config.mjs (1 hunks)
  • packages/rollup-plugin/tsconfig.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • .eslintrc.js
  • packages/rollup-plugin/.gitignore
  • packages/rollup-plugin/tsconfig.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: build
  • GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
packages/rollup-plugin/rollup.config.mjs (2)

14-14: Verify TypeScript configuration existence.

The configuration references ./tsconfig.json. Let's verify its existence and configuration.

Also applies to: 25-25

✅ Verification successful

TypeScript configuration verified and valid

The tsconfig.json exists and contains appropriate TypeScript configuration, extending from @tsconfig/node16 with proper module resolution and declaration file generation settings.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if tsconfig exists and validate its content
ls -la ./packages/rollup-plugin/tsconfig.json
cat ./packages/rollup-plugin/tsconfig.json | jq .

Length of output: 251


Script:

#!/bin/bash
# View raw content of tsconfig.json
cat ./packages/rollup-plugin/tsconfig.json

Length of output: 282


6-6: Verify TypeScript source file existence.

The configuration references ./src/index.ts as the entry point. Let's verify its existence and structure.

Also applies to: 17-17

✅ Verification successful

Entry point verified successfully.

The TypeScript entry point exists at the specified path and exports the required plugin functionality.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the TypeScript entry point exists and show its structure
ls -la ./packages/rollup-plugin/src/index.ts
ast-grep --pattern 'export $$$' ./packages/rollup-plugin/src/index.ts

Length of output: 205


Script:

#!/bin/bash
# Check file content and try different export patterns
cat ./packages/rollup-plugin/src/index.ts

# Try different export patterns
echo -e "\nChecking default export:"
ast-grep --pattern 'export default $$$'

echo -e "\nChecking named exports:"
ast-grep --pattern 'export const $$$'

echo -e "\nChecking type exports:"
ast-grep --pattern 'export type $$$'

Length of output: 66203

Comment on lines 4 to 31
export default defineConfig([
{
input: './src/index.ts',
output: {
dir: './dist',
format: 'esm',
entryFileNames: '[name].mjs',
chunkFileNames: 'chunks/[name].mjs',
sourcemap: true,
},
plugins: [typescript({ tsconfig: './tsconfig.json' })],
},
{
input: './src/index.ts',
output: {
dir: './dist',
format: 'commonjs',
entryFileNames: '[name].cjs',
chunkFileNames: 'chunks/[name].cjs',
sourcemap: true,
},
plugins: [typescript({ tsconfig: './tsconfig.json' })],
},
]);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding essential Rollup configuration options.

The configuration could benefit from additional options to improve the build output:

  1. Add preserveModules: true for better tree-shaking support
  2. Configure external to exclude dependencies from the bundle

Apply this diff to enhance the configuration:

 export default defineConfig([
   {
     input: './src/index.ts',
+    external: [/node_modules/],
     output: {
       dir: './dist',
       format: 'esm',
       entryFileNames: '[name].mjs',
       chunkFileNames: 'chunks/[name].mjs',
       sourcemap: true,
+      preserveModules: true,
     },
     plugins: [typescript({ tsconfig: './tsconfig.json' })],
   },
   {
     input: './src/index.ts',
+    external: [/node_modules/],
     output: {
       dir: './dist',
       format: 'commonjs',
       entryFileNames: '[name].cjs',
       chunkFileNames: 'chunks/[name].cjs',
       sourcemap: true,
+      preserveModules: true,
     },
     plugins: [typescript({ tsconfig: './tsconfig.json' })],
   },
 ]);
📝 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.

Suggested change
export default defineConfig([
{
input: './src/index.ts',
output: {
dir: './dist',
format: 'esm',
entryFileNames: '[name].mjs',
chunkFileNames: 'chunks/[name].mjs',
sourcemap: true,
},
plugins: [typescript({ tsconfig: './tsconfig.json' })],
},
{
input: './src/index.ts',
output: {
dir: './dist',
format: 'commonjs',
entryFileNames: '[name].cjs',
chunkFileNames: 'chunks/[name].cjs',
sourcemap: true,
},
plugins: [typescript({ tsconfig: './tsconfig.json' })],
},
]);
export default defineConfig([
{
input: './src/index.ts',
external: [/node_modules/],
output: {
dir: './dist',
format: 'esm',
entryFileNames: '[name].mjs',
chunkFileNames: 'chunks/[name].mjs',
sourcemap: true,
preserveModules: true,
},
plugins: [typescript({ tsconfig: './tsconfig.json' })],
},
{
input: './src/index.ts',
external: [/node_modules/],
output: {
dir: './dist',
format: 'commonjs',
entryFileNames: '[name].cjs',
chunkFileNames: 'chunks/[name].cjs',
sourcemap: true,
preserveModules: true,
},
plugins: [typescript({ tsconfig: './tsconfig.json' })],
},
]);

@@ -2,8 +2,16 @@
"name": "rollup-plugin-bundle-stats",
"version": "4.18.0",
"description": "In-depth bundle analyzer for rollup(bundle size, assets, modules, packages)",
"main": "lib/index.js",
"main": "./dist/index.cjs",
"module": "./dist/index.mjs",
"types": "types/index.d.ts",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Update types field to match TypeScript output directory

The types field in package.json points to types/index.d.ts, but TypeScript is configured to output declaration files to dist/ directory (declarationDir: "./dist" in tsconfig.json). Update the types field to "dist/index.d.ts" to match the TypeScript configuration and the exports field.

🔗 Analysis chain

Verify TypeScript declaration file output location.

The types field points to types/index.d.ts but exports use ./dist/index.d.ts. Let's verify the TypeScript configuration to ensure declaration files are output to the correct location.

Also applies to: 12-12

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check TypeScript configuration for declaration output
cat ./packages/rollup-plugin/tsconfig.json | jq '.compilerOptions.declarationDir'

Length of output: 146


Script:

#!/bin/bash
# Check if tsconfig exists and view its contents
echo "=== Checking tsconfig.json existence and contents ==="
fd tsconfig.json packages/rollup-plugin/ --type f --exec cat {}

echo -e "\n=== Checking for .d.ts files ==="
fd "\.d\.ts$" packages/rollup-plugin/

echo -e "\n=== Viewing package.json exports field ==="
cat packages/rollup-plugin/package.json | grep -A 10 '"exports":'

Length of output: 923

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant