Skip to content

fix: remove console config#39

Merged
xierenyuan merged 4 commits intokmijs:mainfrom
yk2012:fix/remove-console-config
Sep 10, 2025
Merged

fix: remove console config#39
xierenyuan merged 4 commits intokmijs:mainfrom
yk2012:fix/remove-console-config

Conversation

@yk2012
Copy link
Contributor

@yk2012 yk2012 commented Sep 4, 2025

Summary by CodeRabbit

  • Bug Fixes

    • Ensures console statements are reliably removed across bundlers and minifiers.
    • Improves detection of bundler configuration sources for consistent behavior.
    • Automatically falls back to a compatible minifier in environments where the default cannot remove consoles.
    • Aligns configuration handling for console removal options to work consistently with different minifiers.
  • Tests

    • Adds comprehensive end-to-end tests covering enabled, disabled, and selective console removal.
    • Verifies behavior with multiple JavaScript minifiers.

@coderabbitai
Copy link

coderabbitai bot commented Sep 4, 2025

Walkthrough

Adds a refined removeConsole implementation in preset-bundler with minifier detection/option handling changes, plus comprehensive E2E tests covering basic, disabled, selective, and different minifier scenarios (esbuild, terser). Introduces test configs (.umirc.ts), pages with console calls, and assertions verifying build outputs.

Changes

Cohort / File(s) Summary
Core feature logic
packages/preset-bundler/src/features/removeConsole/removeConsole.ts
Reworks Rspack detection; adjusts default minifier selection; adds Rspack+SWC workaround by switching to Terser; unifies/composes compress options; updates esbuild/terser option mapping; minor message wrap in onCheckConfig.
E2E configs
e2e/test-cases/remove-console/basic/.umirc.ts, .../disabled/.umirc.ts, .../selective/.umirc.ts, .../with-different-minifiers/esbuild/.umirc.ts, .../with-different-minifiers/terser/.umirc.ts
Adds Umi configs for scenarios: removeConsole true/disabled/selective; sets jsMinifier for esbuild/terser cases; references preset-bundler.
E2E tests
e2e/test-cases/remove-console/basic/__tests__/index.test.ts, .../disabled/__tests__/index.test.ts, .../selective/__tests__/index.test.ts, .../with-different-minifiers/esbuild/__tests__/index.test.ts, .../with-different-minifiers/terser/__tests__/index.test.ts
Introduces Vitest suites that build projects and assert dist JS contains/omits console calls per scenario; also verifies pre-build source contains expected calls.
E2E pages
e2e/test-cases/remove-console/basic/pages/index.tsx, .../disabled/pages/index.tsx, .../selective/pages/index.tsx, .../with-different-minifiers/esbuild/pages/index.tsx, .../with-different-minifiers/terser/pages/index.tsx
Adds React pages emitting various console methods on mount/handlers to exercise removal/retention across scenarios.
E2E package manifest
e2e/test-cases/remove-console/package.json
Adds package metadata, test script, React runtime deps, and dev deps (TypeScript, Vitest).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Dev as Dev/Test Runner
  participant Umi as Umi Service
  participant PB as preset-bundler
  participant RC as removeConsole feature
  participant Bundler as Bundler (Rspack/Other)
  participant Min as Minifier (esbuild/terser/swc)

  Dev->>Umi: umi build
  Umi->>PB: load presets
  PB->>RC: configure removeConsole
  RC->>RC: Detect isRspack (memo.rspack or userConfig.rspack)
  alt Rspack detected
    RC->>RC: Default jsMinifier = swc
    opt removeConsole enabled with swc
      Note over RC: Switch to terser (workaround for drop_console)
      RC->>RC: Merge compressOptions (drop_console etc.)
    end
  else Non-Rspack
    RC->>RC: Default jsMinifier = esbuild
    RC->>RC: Map options to esbuild (pure/drop)
  end
  RC->>PB: Return jsMinifier + options
  PB->>Bundler: Apply minifier config
  Bundler->>Min: Minify with compress options
  Min-->>Bundler: Outputs without selected console calls
  Bundler-->>Umi: Emit assets
  Umi-->>Dev: dist/*
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Poem

A bunny taps the keys with grace,
Snips the logs without a trace.
Esbuild, Terser, hop in line—
Rspack’s quirks? We redefine.
Now the console’s quiet glen,
Leaves just code to shine again. 🐇✨

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ 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.
    • 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.
  • 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 the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore or @coderabbit 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

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • 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

@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: 7

🧹 Nitpick comments (23)
e2e/test-cases/remove-console/package.json (2)

6-8: Prefer non-watch test mode for CI stability

Use vitest run to avoid accidental watch mode in some environments.

   "scripts": {
-    "test": "vitest"
+    "test": "vitest run"
   },

13-18: Add Node types for TS ergonomics

Tests/fixtures often use Node APIs; include @types/node to prevent TS annoyances.

   "devDependencies": {
+    "@types/node": "^18.0.0",
     "@types/react": "^18.0.0",
     "@types/react-dom": "^18.0.0",
     "typescript": "^5.0.0",
     "vitest": "^1.0.0"
   }
e2e/test-cases/remove-console/disabled/.umirc.ts (1)

1-5: Optional: use defineConfig for type-safety

Umi’s defineConfig helps catch typos (e.g., option names) at author time.

-export default {
+import { defineConfig } from 'umi'
+export default defineConfig({
   presets: [require.resolve('../../../../packages/preset-bundler')],
   rspack: {},
   // removeConsole is not configured - console statements should be kept
-}
+})
e2e/test-cases/remove-console/basic/.umirc.ts (1)

1-5: Optional: wrap with defineConfig for better DX

Same as the disabled case; adds typing to removeConsole.

-export default {
+import { defineConfig } from 'umi'
+export default defineConfig({
   presets: [require.resolve('../../../../packages/preset-bundler')],
   rspack: {},
   removeConsole: true,
-}
+})
e2e/test-cases/remove-console/basic/pages/index.tsx (1)

1-1: Remove unused default React import and silence no-console lint in fixture

Lean import and prevent lint noise in a test fixture intentionally using console.

-import React, { useEffect } from 'react'
+/* eslint-disable no-console */
+import { useEffect } from 'react'
e2e/test-cases/remove-console/disabled/pages/index.tsx (1)

1-1: Same: trim default React import and disable no-console for the fixture

Keeps fixtures focused and lint-clean.

-import React, { useEffect } from 'react'
+/* eslint-disable no-console */
+import { useEffect } from 'react'
e2e/test-cases/remove-console/basic/__tests__/index.test.ts (3)

5-13: Add generous test timeout to avoid CI flakiness during build.

Builds can exceed Vitest’s default timeout on CI.

-test('removeConsole: true should remove all console statements', async () => {
+test(
+  'removeConsole: true should remove all console statements',
+  { timeout: 120_000 },
+  async () => {
     const cwd = path.join(__dirname, '..')
     const service = createUmi(cwd)

     await service.run({ name: 'build' })
-})
+  },
+)

21-43: DRY the console-method assertions.

Reduces repetition and makes future changes easier.

-  // 验证所有console语句都被移除
-  for (const [fileName, content] of jsFiles) {
-    expect(
-      content,
-      `File ${fileName} should not contain console.log`,
-    ).not.toContain('console.log')
-    expect(
-      content,
-      `File ${fileName} should not contain console.warn`,
-    ).not.toContain('console.warn')
-    expect(
-      content,
-      `File ${fileName} should not contain console.error`,
-    ).not.toContain('console.error')
-    expect(
-      content,
-      `File ${fileName} should not contain console.info`,
-    ).not.toContain('console.info')
-    expect(
-      content,
-      `File ${fileName} should not contain console.debug`,
-    ).not.toContain('console.debug')
-  }
+  // 验证所有console语句都被移除
+  const methods = ['log', 'warn', 'error', 'info', 'debug'] as const
+  for (const [fileName, content] of jsFiles) {
+    for (const m of methods) {
+      expect(content, `File ${fileName} should not contain console.${m}`).not.toContain(
+        `console.${m}`,
+      )
+    }
+  }

46-56: Prefer ESM import for fs and align pre-build checks with post-build assertions.

Use import fs from 'node:fs' for consistency; also consider asserting console.debug here or drop the debug check above for symmetry.

Would you like me to update the test page to include a console.debug so both tests align?

e2e/test-cases/remove-console/disabled/__tests__/index.test.ts (3)

5-13: Add generous test timeout to avoid CI flakiness during build.

-test('without removeConsole config - all console statements should be kept', async () => {
+test(
+  'without removeConsole config - all console statements should be kept',
+  { timeout: 120_000 },
+  async () => {
     const cwd = path.join(__dirname, '..')
     const service = createUmi(cwd)

     await service.run({ name: 'build' })
-})
+  },
+)

21-35: DRY the console-method assertions.

-  // 验证所有console语句都被保留
-  for (const [fileName, content] of jsFiles) {
-    expect(content, `File ${fileName} should contain console.log`).toContain(
-      'console.log',
-    )
-    expect(content, `File ${fileName} should contain console.warn`).toContain(
-      'console.warn',
-    )
-    expect(content, `File ${fileName} should contain console.error`).toContain(
-      'console.error',
-    )
-    expect(content, `File ${fileName} should contain console.info`).toContain(
-      'console.info',
-    )
-  }
+  // 验证所有console语句都被保留
+  const methods = ['log', 'warn', 'error', 'info'] as const
+  for (const [fileName, content] of jsFiles) {
+    for (const m of methods) {
+      expect(content, `File ${fileName} should contain console.${m}`).toContain(
+        `console.${m}`,
+      )
+    }
+  }

38-47: Prefer ESM import for fs and (optionally) assert console.debug for symmetry with “basic” test.

Do you want me to also include a console.debug in the disabled fixture and assertion for parity?

packages/preset-bundler/src/features/removeConsole/removeConsole.ts (3)

37-57: Silent minifier switch could surprise users; emit a warning.

Auto-switching SWC→terser is sensible, but add a warning so users understand why the minifier changed.

     if (isRspack && jsMinifier === 'swc') {
-      // SWC configuration issue: drop_console doesn't work in current Rspack version
-      // Workaround: Automatically switch to terser when removeConsole is enabled
+      // SWC configuration issue: drop_console doesn't work in current Rspack version
+      // Workaround: Automatically switch to terser when removeConsole is enabled
+      api.logger?.warn?.(
+        '[removeConsole] Rspack+swc does not support console removal; switching jsMinifier to "terser".',
+      )

59-62: Avoid duplicate compressOptions declarations.

Define once and reuse to reduce noise.

-    const compressOptions = Array.isArray(removeConsole)
-      ? { pure_funcs: removeConsole.map((method) => `console.${method}`) }
-      : { drop_console: true }
+    const compressOptions =
+      Array.isArray(removeConsole)
+        ? { pure_funcs: removeConsole.map((m) => `console.${m}`) }
+        : { drop_console: true }

17-26: Minor: keep detection logic consistent across hooks.

onCheckConfig uses api.appData.bundler while modifyConfig used config-based detection earlier. After applying the unified isRspack, consider reusing it here for clarity.

e2e/test-cases/remove-console/with-different-minifiers/terser/__tests__/index.test.ts (1)

21-39: Prefer regex over substring checks for robustness.

Regex avoids matching within unrelated strings and covers minifier formatting. Optional if you prefer current style.

-  for (const [fileName, content] of jsFiles) {
-    expect(
-      content,
-      `File ${fileName} should not contain console.log`,
-    ).not.toContain('console.log')
-    expect(
-      content,
-      `File ${fileName} should not contain console.warn`,
-    ).not.toContain('console.warn')
-    expect(
-      content,
-      `File ${fileName} should not contain console.error`,
-    ).not.toContain('console.error')
-    expect(
-      content,
-      `File ${fileName} should not contain console.info`,
-    ).not.toContain('console.info')
-  }
+  const pattern = /\bconsole\.(log|warn|error|info|debug)\s*\(/;
+  for (const [fileName, content] of jsFiles) {
+    expect(content, `File ${fileName} should not contain console.* calls`).not.toMatch(pattern)
+  }
e2e/test-cases/remove-console/with-different-minifiers/terser/.umirc.ts (1)

3-3: Drop empty rspack block (minor).

If not used by this test, remove to keep the fixture minimal.

-  rspack: {},
e2e/test-cases/remove-console/with-different-minifiers/esbuild/__tests__/index.test.ts (1)

21-39: De-duplicate assertions with a shared helper (optional).

Both minifier tests share identical logic. Consider extracting to @e2e/helper (e.g., assertNoConsole(distPath)).

Example helper (TypeScript) to add in @e2e/helper:

export async function assertNoConsole(distPath: string) {
  const files = await unwrapOutputJSON(distPath)
  const jsFiles = Object.entries(files).filter(([n]) => n.endsWith('.js') || n.endsWith('.mjs'))
  expect(jsFiles.length).toBeGreaterThan(0)
  const pattern = /\bconsole\.(log|warn|error|info|debug)\s*\(/;
  for (const [fileName, content] of jsFiles) {
    expect(content, `File ${fileName} should not contain console.* calls`).not.toMatch(pattern)
  }
}
e2e/test-cases/remove-console/selective/pages/index.tsx (1)

25-27: Clarify copy to include debug for consistency.

You keep debug as well; reflect that in the description to avoid confusion.

-      <p>Only log and warn should be removed, error and info should be kept</p>
+      <p>Only log and warn should be removed; error, info, and debug should be kept</p>
e2e/test-cases/remove-console/with-different-minifiers/esbuild/pages/index.tsx (2)

1-1: Drop unused default React import.

Not using the React identifier; keep only useEffect.

-import React, { useEffect } from 'react'
+import { useEffect } from 'react'

24-27: Unify “esbuild” casing in UI copy.

Currently mixed: “ESBuild” (title) vs “esbuild” (text). Pick one (typically “esbuild”).

-      <h1>Remove Console Test - ESBuild Minifier</h1>
+      <h1>Remove Console Test - esbuild Minifier</h1>
-        All console statements should be removed when using esbuild minifier
+        All console statements should be removed when using esbuild minifier
e2e/test-cases/remove-console/selective/__tests__/index.test.ts (2)

32-38: Also assert console.debug is preserved (if present).

Selective removal keeps methods other than log/warn. Optional but increases confidence.

     expect(content, `File ${fileName} should contain console.info`).toContain(
       'console.info',
     )
+    // If the source contains console.debug, it should be preserved too.
+    if (content.includes('console.debug') || content.includes('console["debug"]')) {
+      expect(content, `File ${fileName} should contain console.debug`).toContain(
+        'console.debug',
+      )
+    }

42-52: Use ESM import for fs and verify console.debug exists pre-build.

Keep module style consistent and ensure the page truly exercises the “debug” path.

-test('should verify test page contains all console statements before build', () => {
+test('should verify test page contains all console statements before build', () => {
   const testPagePath = path.join(__dirname, '../pages/index.tsx')
-  const fs = require('fs')
-  const content = fs.readFileSync(testPagePath, 'utf-8')
+  const content = fs.readFileSync(testPagePath, 'utf-8')
 
   // 确保测试页面确实包含所有类型的console语句
   expect(content).toContain('console.log')
   expect(content).toContain('console.warn')
   expect(content).toContain('console.error')
   expect(content).toContain('console.info')
+  expect(content).toContain('console.debug')
 })

Add this top-level import outside the shown range:

import fs from 'node:fs'
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 55b9b0c and b31f93b.

📒 Files selected for processing (17)
  • e2e/test-cases/remove-console/basic/.umirc.ts (1 hunks)
  • e2e/test-cases/remove-console/basic/__tests__/index.test.ts (1 hunks)
  • e2e/test-cases/remove-console/basic/pages/index.tsx (1 hunks)
  • e2e/test-cases/remove-console/disabled/.umirc.ts (1 hunks)
  • e2e/test-cases/remove-console/disabled/__tests__/index.test.ts (1 hunks)
  • e2e/test-cases/remove-console/disabled/pages/index.tsx (1 hunks)
  • e2e/test-cases/remove-console/package.json (1 hunks)
  • e2e/test-cases/remove-console/selective/.umirc.ts (1 hunks)
  • e2e/test-cases/remove-console/selective/__tests__/index.test.ts (1 hunks)
  • e2e/test-cases/remove-console/selective/pages/index.tsx (1 hunks)
  • e2e/test-cases/remove-console/with-different-minifiers/esbuild/.umirc.ts (1 hunks)
  • e2e/test-cases/remove-console/with-different-minifiers/esbuild/__tests__/index.test.ts (1 hunks)
  • e2e/test-cases/remove-console/with-different-minifiers/esbuild/pages/index.tsx (1 hunks)
  • e2e/test-cases/remove-console/with-different-minifiers/terser/.umirc.ts (1 hunks)
  • e2e/test-cases/remove-console/with-different-minifiers/terser/__tests__/index.test.ts (1 hunks)
  • e2e/test-cases/remove-console/with-different-minifiers/terser/pages/index.tsx (1 hunks)
  • packages/preset-bundler/src/features/removeConsole/removeConsole.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
e2e/test-cases/remove-console/disabled/pages/index.tsx (4)
e2e/test-cases/remove-console/basic/pages/index.tsx (1)
  • HomePage (3-30)
e2e/test-cases/remove-console/selective/pages/index.tsx (1)
  • HomePage (3-31)
e2e/test-cases/remove-console/with-different-minifiers/esbuild/pages/index.tsx (1)
  • HomePage (3-32)
e2e/test-cases/remove-console/with-different-minifiers/terser/pages/index.tsx (1)
  • HomePage (3-30)
e2e/test-cases/remove-console/with-different-minifiers/esbuild/pages/index.tsx (2)
e2e/test-cases/remove-console/basic/pages/index.tsx (1)
  • HomePage (3-30)
e2e/test-cases/remove-console/with-different-minifiers/terser/pages/index.tsx (1)
  • HomePage (3-30)
e2e/test-cases/remove-console/with-different-minifiers/terser/pages/index.tsx (1)
e2e/test-cases/remove-console/with-different-minifiers/esbuild/pages/index.tsx (1)
  • HomePage (3-32)
e2e/test-cases/remove-console/with-different-minifiers/esbuild/__tests__/index.test.ts (1)
e2e/helper/helper.ts (2)
  • createUmi (25-31)
  • unwrapOutputJSON (53-58)
e2e/test-cases/remove-console/selective/pages/index.tsx (4)
e2e/test-cases/remove-console/basic/pages/index.tsx (1)
  • HomePage (3-30)
e2e/test-cases/remove-console/disabled/pages/index.tsx (1)
  • HomePage (3-33)
e2e/test-cases/remove-console/with-different-minifiers/esbuild/pages/index.tsx (1)
  • HomePage (3-32)
e2e/test-cases/remove-console/with-different-minifiers/terser/pages/index.tsx (1)
  • HomePage (3-30)
e2e/test-cases/remove-console/with-different-minifiers/terser/__tests__/index.test.ts (1)
e2e/helper/helper.ts (2)
  • createUmi (25-31)
  • unwrapOutputJSON (53-58)
e2e/test-cases/remove-console/disabled/__tests__/index.test.ts (1)
e2e/helper/helper.ts (2)
  • createUmi (25-31)
  • unwrapOutputJSON (53-58)
e2e/test-cases/remove-console/basic/pages/index.tsx (4)
e2e/test-cases/remove-console/disabled/pages/index.tsx (1)
  • HomePage (3-33)
e2e/test-cases/remove-console/selective/pages/index.tsx (1)
  • HomePage (3-31)
e2e/test-cases/remove-console/with-different-minifiers/esbuild/pages/index.tsx (1)
  • HomePage (3-32)
e2e/test-cases/remove-console/with-different-minifiers/terser/pages/index.tsx (1)
  • HomePage (3-30)
e2e/test-cases/remove-console/selective/__tests__/index.test.ts (1)
e2e/helper/helper.ts (2)
  • createUmi (25-31)
  • unwrapOutputJSON (53-58)
e2e/test-cases/remove-console/basic/__tests__/index.test.ts (1)
e2e/helper/helper.ts (2)
  • createUmi (25-31)
  • unwrapOutputJSON (53-58)
packages/preset-bundler/src/features/removeConsole/removeConsole.ts (1)
packages/preset-bundler/src/index.ts (1)
  • api (3-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: build-and-test (22.x, windows-latest)
  • GitHub Check: build-and-test (22.x, ubuntu-latest)
🔇 Additional comments (8)
packages/preset-bundler/src/features/removeConsole/removeConsole.ts (3)

63-73: LGTM on esbuild option mapping.

pure for selective and drop: ['console'] for full removal aligns with esbuild’s API.


76-85: LGTM on terser option mapping.

Merging into compress with pure_funcs/drop_console is correct.


28-87: No downstream modifyConfig overrides jsMinifier

e2e/test-cases/remove-console/with-different-minifiers/terser/.umirc.ts (1)

3-5: Config looks correct for terser + removeConsole.

No issues spotted.

e2e/test-cases/remove-console/selective/pages/index.tsx (1)

1-31: Component content aligned with selective behavior.

Matches removeConsole: ['log', 'warn'] expectations.

e2e/test-cases/remove-console/with-different-minifiers/terser/pages/index.tsx (1)

1-30: Fixture content is appropriate for terser scenario.

No issues spotted.

e2e/test-cases/remove-console/selective/.umirc.ts (1)

1-5: Config looks good for selective removal.

Matches the intended behavior to drop only console.log and console.warn.

e2e/test-cases/remove-console/with-different-minifiers/esbuild/.umirc.ts (1)

4-5: No action needed: 'esbuild' is a supported jsMinifier value in preset-bundler.

Comment on lines 15 to 17
const jsFiles = Object.entries(files).filter(
([name]) => name.endsWith('.js') && name.includes('umi'),
)
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Don’t rely on 'umi' naming; scan all JS assets.

Same concern as the terser test—vendors/runtime may be missed.

-  const jsFiles = Object.entries(files).filter(
-    ([name]) => name.endsWith('.js') && name.includes('umi'),
-  )
+  const jsFiles = Object.entries(files).filter(
+    ([name]) => name.endsWith('.js') || name.endsWith('.mjs'),
+  )
📝 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 jsFiles = Object.entries(files).filter(
([name]) => name.endsWith('.js') && name.includes('umi'),
)
const jsFiles = Object.entries(files).filter(
([name]) => name.endsWith('.js') || name.endsWith('.mjs'),
)
🤖 Prompt for AI Agents
In
e2e/test-cases/remove-console/with-different-minifiers/esbuild/__tests__/index.test.ts
around lines 15 to 17, the test filters JS files by name.includes('umi') which
can miss vendor/runtime assets; change the selection to include all JS assets
(e.g., only filter by name.endsWith('.js')) so the test scans every JavaScript
output, and update any subsequent assertions/expectations to iterate over that
full set instead of assuming 'umi' files only.

Comment on lines 21 to 39
// 验证所有console语句都被移除
for (const [fileName, content] of jsFiles) {
expect(
content,
`File ${fileName} should not contain console.log`,
).not.toContain('console.log')
expect(
content,
`File ${fileName} should not contain console.warn`,
).not.toContain('console.warn')
expect(
content,
`File ${fileName} should not contain console.error`,
).not.toContain('console.error')
expect(
content,
`File ${fileName} should not contain console.info`,
).not.toContain('console.info')
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Also assert removal of console.debug.

The page emits console.debug but the test doesn’t check it.

     expect(
       content,
       `File ${fileName} should not contain console.info`,
     ).not.toContain('console.info')
+    expect(
+      content,
+      `File ${fileName} should not contain console.debug`,
+    ).not.toContain('console.debug')
📝 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
// 验证所有console语句都被移除
for (const [fileName, content] of jsFiles) {
expect(
content,
`File ${fileName} should not contain console.log`,
).not.toContain('console.log')
expect(
content,
`File ${fileName} should not contain console.warn`,
).not.toContain('console.warn')
expect(
content,
`File ${fileName} should not contain console.error`,
).not.toContain('console.error')
expect(
content,
`File ${fileName} should not contain console.info`,
).not.toContain('console.info')
}
// 验证所有console语句都被移除
for (const [fileName, content] of jsFiles) {
expect(
content,
`File ${fileName} should not contain console.log`,
).not.toContain('console.log')
expect(
content,
`File ${fileName} should not contain console.warn`,
).not.toContain('console.warn')
expect(
content,
`File ${fileName} should not contain console.error`,
).not.toContain('console.error')
expect(
content,
`File ${fileName} should not contain console.info`,
).not.toContain('console.info')
expect(
content,
`File ${fileName} should not contain console.debug`,
).not.toContain('console.debug')
}
🤖 Prompt for AI Agents
In
e2e/test-cases/remove-console/with-different-minifiers/terser/__tests__/index.test.ts
around lines 21 to 39, the test verifies removal of console.log/warn/error/info
but omits console.debug; update the assertions to also assert that each file's
content does not contain 'console.debug' by adding an expect similar to the
others (with the same File ${fileName} should not contain console.debug message)
inside the loop so console.debug occurrences are checked and cause the test to
fail if present.

? { pure_funcs: removeConsole.map((method) => `console.${method}`) }
: { drop_console: true }
// Fix: Check for rspack config instead of bundler name
const isRspack = !!memo.rspack || !!api.userConfig.rspack
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Unify Rspack detection with appData to avoid false positives.

!!api.userConfig.rspack can be set even when bundler isn’t Rspack, causing misclassification. Prefer api.appData.bundler with a fallback.

-    const isRspack = !!memo.rspack || !!api.userConfig.rspack
+    const isRspack =
+      api.appData.bundler === 'rspack' || Boolean(memo.rspack)
📝 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 isRspack = !!memo.rspack || !!api.userConfig.rspack
const isRspack =
api.appData.bundler === 'rspack' || Boolean(memo.rspack)
🤖 Prompt for AI Agents
In packages/preset-bundler/src/features/removeConsole/removeConsole.ts around
line 31, the current Rspack detection uses !!api.userConfig.rspack which can
misclassify projects; change the logic to prefer api.appData.bundler (e.g.
api.appData?.bundler === 'rspack') and fall back to memo.rspack if appData is
unavailable, removing reliance on userConfig.rspack to avoid false positives.

@xierenyuan xierenyuan changed the title Fix/remove console config fix: remove console config Sep 10, 2025
@xierenyuan xierenyuan merged commit 6f2b2cc into kmijs:main Sep 10, 2025
4 checks passed
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.

2 participants