Webpack: Improve performance of module-mocking plugins#33169
Conversation
|
View your CI Pipeline Execution ↗ for commit a0e3f49
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughAdds lazy caching and duplicate-asset guards to the mocker runtime injector; adds mtime-based invalidation and candidate-specifier tracking to the mock extraction/replace plugin to avoid unnecessary re-extraction and module replacements. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
✨ Finishing touches
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Failure to add the new IP will result in interrupted reviews. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/builders/builder-webpack5/src/plugins/webpack-inject-mocker-runtime-plugin.ts (1)
12-13: Minor typo in comment."includiweng" should be "including".
- * This ensures the `sb` object is available globally before any other scripts, includiweng the + * This ensures the `sb` object is available globally before any other scripts, including the
🧹 Nitpick comments (2)
code/builders/builder-webpack5/src/plugins/webpack-inject-mocker-runtime-plugin.ts (1)
62-72: Consider guardingemitAssetto avoid duplicate asset warnings.The
includescheck on line 70 prevents duplicate entries indata.assets.js, butemitAssetis called unconditionally on every hook invocation. In watch mode or with multiple HTML files, this could attempt to emit an asset that already exists, potentially causing warnings or errors.Consider checking if the asset exists before emitting:
- compilation.emitAsset( - runtimeAssetName, - new compiler.webpack.sources.RawSource(runtimeScriptContent) - ); + if (!compilation.getAsset(runtimeAssetName)) { + compilation.emitAsset( + runtimeAssetName, + new compiler.webpack.sources.RawSource(runtimeScriptContent) + ); + }code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts (1)
136-144: Consider adding type safety and debug logging for filesystem access.The
as anycast loses type safety. While the defensive optional chaining is appropriate (not all webpack file systems implementstatSync), a debug log on failure would aid troubleshooting.private getPreviewConfigMtime(compiler: Compiler): number | undefined { + const logger = compiler.getInfrastructureLogger(PLUGIN_NAME); try { - const fs = compiler.inputFileSystem as any; + const fs = compiler.inputFileSystem as { statSync?: (path: string) => { mtime?: Date } }; const stat = fs.statSync?.(this.options.previewConfigPath); return stat?.mtime?.getTime?.(); - } catch { + } catch (e) { + logger.debug(`Could not stat preview config: ${e}`); return undefined; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/builders/builder-webpack5/src/plugins/webpack-inject-mocker-runtime-plugin.ts(3 hunks)code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use camelCase for variable and function names
Files:
code/builders/builder-webpack5/src/plugins/webpack-inject-mocker-runtime-plugin.tscode/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Enable TypeScript strict mode
Export functions from modules for testing purposes
Files:
code/builders/builder-webpack5/src/plugins/webpack-inject-mocker-runtime-plugin.tscode/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
**/*.{ts,tsx,js,jsx,json,html,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx,json,html,mjs}: Use ESLint and Prettier for code style enforcement
Run 'yarn prettier --write ' to format code after making changes
Run 'yarn lint:js:cmd ' to check for ESLint issues after making changes
Files:
code/builders/builder-webpack5/src/plugins/webpack-inject-mocker-runtime-plugin.tscode/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
code/**/!(*.test).{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/!(*.test).{ts,tsx,js,mjs}: Use 'logger' from 'storybook/internal/node-logger' for server-side (Node.js) logging, not console.log/console.warn/console.error
Use 'logger' from 'storybook/internal/client-logger' for client-side (browser) logging, not console.log/console.warn/console.error
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/builders/builder-webpack5/src/plugins/webpack-inject-mocker-runtime-plugin.tscode/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
🧠 Learnings (15)
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies in Vitest tests
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:47.649Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T17:49:47.649Z
Learning: Applies to **/*.test.{ts,tsx} : Use 'vi.mock()' to mock external dependencies like file system and loggers in unit tests
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases in Vitest tests
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required dependencies that the test subject uses
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock implementations should match the expected return type of the original function
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `vi.mock()` with the `spy: true` option for all package and file mocks in Vitest tests
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors in Vitest tests
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid direct function mocking without `vi.mocked()` in Vitest tests
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Keep mock implementations simple and focused in Vitest tests
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use type-safe mocking with `vi.mocked()` in Vitest tests
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking without the `spy: true` option in Vitest tests
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mock implementations outside of `beforeEach` blocks in Vitest tests
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in `beforeEach` blocks in Vitest tests
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Place all mocks at the top of the test file before any test cases
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Each mock implementation should return a Promise for async functions in Vitest
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
🧬 Code graph analysis (2)
code/builders/builder-webpack5/src/plugins/webpack-inject-mocker-runtime-plugin.ts (1)
code/core/src/mocking-utils/runtime.ts (1)
getMockerRuntime(13-28)
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts (1)
test-storybooks/portable-stories-kitchen-sink/react-vitest-3/pre-e2e.js (1)
fs(3-3)
⏰ 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: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (7)
code/builders/builder-webpack5/src/plugins/webpack-inject-mocker-runtime-plugin.ts (3)
16-16: LGTM on the caching approach.Caching the runtime script avoids repeated
esbuild.buildSynccalls on each compilation, which is a good performance optimization given thatgetMockerRuntime()bundles synchronously.
56-57: Good use of lazy initialization pattern.The nullish coalescing assignment (
??=semantics via?? (this.cachedRuntime = ...)) ensures the runtime is only generated once per plugin instance.
74-76: Good fix for error propagation.Previously, exceptions would leave the async callback hanging. Now the error is properly forwarded to webpack's compilation pipeline via
cb(error as Error).code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts (4)
52-53: LGTM on the new state fields.The
candidateSpecifiersset enables O(1) lookup for filtering irrelevant module requests, andlastPreviewMtimeenables the mtime-based cache invalidation. Both are well-typed.
72-79: Effective mtime-based invalidation logic.The early return when
mTimePreviewConfig <= lastPreviewMtimecorrectly avoids redundant re-extraction. The fallback behavior whenmTimePreviewConfigisundefined(re-extract) is also correct for robustness.
80-89: LGTM on the mock map construction.Using
flatMapto register both the full path and extension-stripped path enables matching regardless of how the importer specifies the extension. UpdatingcandidateSpecifiersand logging the count improves observability.
98-107: Remove the early guard check or add path normalization to handle aliased/mapped imports.The guard at line 105 returns early if
resource.requestis not incandidateSpecifiers, which stores the originalsb.mock()paths. However, this check is too strict and blocks valid mocks when:
- Imports use path aliases or tsconfig mappings that differ from how
sb.mock()was called- Webpack normalizes paths differently (e.g.,
./file.tsvs./file)The fallback resolution logic (lines 109–115) could handle these cases via
resolveWithExtensions()andmockMaplookup by absolute path, but it's never reached due to the early return.Options:
- Remove the early guard to let all imports attempt resolution
- Store normalized/resolved paths in
candidateSpecifiersalongside original paths- Normalize both
resource.requestand candidate paths before comparison
| logger.info(`Mock map updated with ${this.mockMap.size / 2} mocks.`); | ||
| this.candidateSpecifiers = new Set(resolved.map((m) => m.path)); | ||
| this.lastPreviewMtime = mTimePreviewConfig; | ||
| logger.info(`Mock map updated with ${resolved.length} mocks.`); |
There was a problem hiding this comment.
little reminder, now that you're touching this file, to either:
- not show the log when the count is 0
- only show the log if the current count is 0 and previous count wasn't 0 (so you don't keep showing "updated 0 mocks" every time)
valentinpalkovic
force-pushed
the
valentin/fix-hmr-for-webpack-users
branch
from
0ede16e to
ce88eed
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts (1)
71-88: Verify mtime behavior during initial compilation.The mtime-based invalidation logic looks sound. However, consider this edge case: during the initial compilation (
beforeRun),this.lastPreviewMtimeisundefined, so the check on line 73 will be false and extraction will proceed correctly.On subsequent watch compilations, if the preview config hasn't changed, the early return on line 77 will skip re-extraction as intended.
Regarding line 88: As noted in the past review comment, consider either:
- Not showing the log when
resolved.lengthis 0- Only showing when transitioning from non-zero to zero
To verify the mtime comparison works correctly across different file systems and scenarios, run:
#!/bin/bash # Verify that inputFileSystem.statSync is available in webpack rg -n "inputFileSystem" --type=ts -C3 -g '!node_modules'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/builders/builder-webpack5/src/plugins/webpack-inject-mocker-runtime-plugin.ts(3 hunks)code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/builders/builder-webpack5/src/plugins/webpack-inject-mocker-runtime-plugin.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use camelCase for variable and function names
Files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Enable TypeScript strict mode
Export functions from modules for testing purposes
Files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
**/*.{ts,tsx,js,jsx,json,html,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx,json,html,mjs}: Use ESLint and Prettier for code style enforcement
Run 'yarn prettier --write ' to format code after making changes
Run 'yarn lint:js:cmd ' to check for ESLint issues after making changes
Files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
code/**/!(*.test).{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/!(*.test).{ts,tsx,js,mjs}: Use 'logger' from 'storybook/internal/node-logger' for server-side (Node.js) logging, not console.log/console.warn/console.error
Use 'logger' from 'storybook/internal/client-logger' for client-side (browser) logging, not console.log/console.warn/console.error
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
🧠 Learnings (15)
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies in Vitest tests
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases in Vitest tests
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:47.649Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T17:49:47.649Z
Learning: Applies to **/*.test.{ts,tsx} : Use 'vi.mock()' to mock external dependencies like file system and loggers in unit tests
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `vi.mock()` with the `spy: true` option for all package and file mocks in Vitest tests
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking without the `spy: true` option in Vitest tests
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock implementations should match the expected return type of the original function
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mock implementations outside of `beforeEach` blocks in Vitest tests
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required dependencies that the test subject uses
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid direct function mocking without `vi.mocked()` in Vitest tests
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors in Vitest tests
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:47.649Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T17:49:47.649Z
Learning: Applies to code/**/!(*.test).{ts,tsx,js,mjs} : Use 'logger' from 'storybook/internal/node-logger' for server-side (Node.js) logging, not console.log/console.warn/console.error
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:47.649Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T17:49:47.649Z
Learning: Applies to code/**/!(*.test).{ts,tsx,js,mjs} : Use 'logger' from 'storybook/internal/client-logger' for client-side (browser) logging, not console.log/console.warn/console.error
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses in Vitest tests
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:47.649Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T17:49:47.649Z
Learning: Applies to **/*.{ts,tsx} : Export functions from modules for testing purposes
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use type-safe mocking with `vi.mocked()` in Vitest tests
Applied to files:
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts
🧬 Code graph analysis (1)
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts (1)
code/core/src/mocking-utils/resolve.ts (2)
isExternal(112-118)getIsExternal(58-64)
⏰ 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: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (4)
code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts (4)
11-11: LGTM! Correct logger import.The import follows coding guidelines by using the node-logger for server-side logging in this webpack plugin.
53-54: LGTM! Well-designed optimization state.These private fields support mtime-based invalidation and early filtering, improving performance by avoiding unnecessary mock re-extraction and module resolution.
107-109: LGTM! Smart early-exit optimization.The logic correctly filters external modules that aren't being mocked, while still allowing relative/local paths to proceed to full resolution. This is necessary because external specifiers (like
'lodash') are consistent across all imports, but relative specifiers (like'../utils/api') vary by importer location.
136-144: I'll verify the review comment by investigating webpack's type definitions forinputFileSystemand assessing the type safety concern.
<function_calls>
#!/bin/bashFirst, check the file to understand the context
cat -n code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts | sed -n '130,150p'
</function_calls>
<function_calls>
#!/bin/bashCheck webpack version in package.json
fd -n "package.json" code/builders/builder-webpack5 | head -1 | xargs cat | grep -A2 -B2 '"webpack'
</function_calls>
<function_calls>
#!/bin/bashSearch for InputFileSystem type in webpack or the file itself
rg -n "InputFileSystem|inputFileSystem" code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts -C5
</function_calls>
<function_calls>
web_search for webpack InputFileSystem type interface definition
</function_calls>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/builders/builder-webpack5/src/plugins/webpack-inject-mocker-runtime-plugin.ts (1)
10-14: Fix typo in JSDoc comment."includiweng" should be "including".
/** * A Webpack plugin that injects the module mocker runtime script into the preview iframe's HTML. - * This ensures the `sb` object is available globally before any other scripts, includiweng the + * This ensures the `sb` object is available globally before any other scripts, including the * Storybook preview bundle, are executed. */
🧹 Nitpick comments (1)
code/builders/builder-webpack5/src/plugins/webpack-inject-mocker-runtime-plugin.ts (1)
62-73: Guard logic is correct; consider relocating the comment for clarity.The asset emission guard correctly prevents duplicates. However, the comment on lines 70-72 describes the
unshiftoperation inside the if block but is positioned outside it, making it appear to describe the callback instead.if (!compilation.getAsset(runtimeAssetName)) { compilation.emitAsset( runtimeAssetName, new compiler.webpack.sources.RawSource(runtimeScriptContent) ); + // Prepend the name of our new asset to the list of JavaScript files. + // HtmlWebpackPlugin will automatically create a <script> tag for it + // and place it at the beginning of the body scripts. data.assets.js.unshift(runtimeAssetName); } - // Prepend the name of our new asset to the list of JavaScript files, once. - // HtmlWebpackPlugin will automatically create a <script> tag for it - // and place it at the beginning of the body scripts. cb(null, data);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/builders/builder-webpack5/src/plugins/webpack-inject-mocker-runtime-plugin.ts(2 hunks)code/builders/builder-webpack5/src/preview/iframe-webpack.config.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use camelCase for variable and function names
Files:
code/builders/builder-webpack5/src/preview/iframe-webpack.config.tscode/builders/builder-webpack5/src/plugins/webpack-inject-mocker-runtime-plugin.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Enable TypeScript strict mode
Export functions from modules for testing purposes
Files:
code/builders/builder-webpack5/src/preview/iframe-webpack.config.tscode/builders/builder-webpack5/src/plugins/webpack-inject-mocker-runtime-plugin.ts
**/*.{ts,tsx,js,jsx,json,html,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx,json,html,mjs}: Use ESLint and Prettier for code style enforcement
Run 'yarn prettier --write ' to format code after making changes
Run 'yarn lint:js:cmd ' to check for ESLint issues after making changes
Files:
code/builders/builder-webpack5/src/preview/iframe-webpack.config.tscode/builders/builder-webpack5/src/plugins/webpack-inject-mocker-runtime-plugin.ts
code/**/!(*.test).{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/!(*.test).{ts,tsx,js,mjs}: Use 'logger' from 'storybook/internal/node-logger' for server-side (Node.js) logging, not console.log/console.warn/console.error
Use 'logger' from 'storybook/internal/client-logger' for client-side (browser) logging, not console.log/console.warn/console.error
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/builders/builder-webpack5/src/preview/iframe-webpack.config.tscode/builders/builder-webpack5/src/plugins/webpack-inject-mocker-runtime-plugin.ts
🪛 ast-grep (0.40.0)
code/builders/builder-webpack5/src/preview/iframe-webpack.config.ts
[warning] 132-132: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${resolvedOutputDir.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')})
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (4)
code/builders/builder-webpack5/src/preview/iframe-webpack.config.ts (2)
47-47: LGTM! Correctly resolves the output directory.The resolved absolute path is needed for the watchOptions.ignored configuration to prevent rebuild loops.
130-134: LGTM! Properly prevents rebuild loops with escaped regex pattern.The change correctly ignores both
node_modulesand the output directory in watch mode. The regex escaping using.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')is the standard pattern for escaping special characters.Regarding the static analysis ReDoS warning: this is a false positive in this context since
resolvedOutputDircomes from developer configuration (theoutputDiroption), not runtime user input. The path is properly escaped and the pattern is straightforward.code/builders/builder-webpack5/src/plugins/webpack-inject-mocker-runtime-plugin.ts (2)
16-16: LGTM!The lazy caching pattern using nullish coalescing with assignment is idiomatic and correctly avoids redundant
getMockerRuntime()calls across compilations.Also applies to: 56-57
74-77: LGTM!Properly forwarding errors to the callback ensures Webpack can handle and report failures during asset tag generation.
valentinpalkovic
force-pushed
the
valentin/fix-hmr-for-webpack-users
branch
from
289b99b to
5c0c277
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
code/builders/builder-webpack5/src/presets/custom-webpack-preset.ts (1)
62-65: Ensure early return if module-mocking is disabled.Since all three module-mocking components are disabled (lines 45-55, 60, 65), the early return at lines 38-40 becomes the only functional guard. However, the function still continues to line 66, setting up empty plugin arrays unnecessarily.
Consider making the early return more explicit:
// If there's no preview file, there's nothing to mock. + // Module-mocking is currently disabled in this preset. if (!previewConfigPath) { return config; } - config.plugins = config.plugins || []; + // config.plugins = config.plugins || []; // 1. Add the loader to normalize sb.mock(import(...)) calls.Or remove the entire function body since it effectively does nothing:
export async function webpackFinal(config: Configuration, options: Options) { - const previewConfigPath = findConfigFile('preview', options.configDir); - - // If there's no preview file, there's nothing to mock. - if (!previewConfigPath) { - return config; - } - - config.plugins = config.plugins || []; - - // ... all commented code ... - + // Module-mocking workflow temporarily disabled (see PR #33169) return config; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/builders/builder-webpack5/src/presets/custom-webpack-preset.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use camelCase for variable and function names
Files:
code/builders/builder-webpack5/src/presets/custom-webpack-preset.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Enable TypeScript strict mode
Export functions from modules for testing purposes
Files:
code/builders/builder-webpack5/src/presets/custom-webpack-preset.ts
**/*.{ts,tsx,js,jsx,json,html,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx,json,html,mjs}: Use ESLint and Prettier for code style enforcement
Run 'yarn prettier --write ' to format code after making changes
Run 'yarn lint:js:cmd ' to check for ESLint issues after making changes
Files:
code/builders/builder-webpack5/src/presets/custom-webpack-preset.ts
code/**/!(*.test).{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/!(*.test).{ts,tsx,js,mjs}: Use 'logger' from 'storybook/internal/node-logger' for server-side (Node.js) logging, not console.log/console.warn/console.error
Use 'logger' from 'storybook/internal/client-logger' for client-side (browser) logging, not console.log/console.warn/console.error
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/builders/builder-webpack5/src/presets/custom-webpack-preset.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases in Vitest tests
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `vi.mock()` with the `spy: true` option for all package and file mocks in Vitest tests
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking without the `spy: true` option in Vitest tests
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use type-safe mocking with `vi.mocked()` in Vitest tests
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required dependencies that the test subject uses
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid direct function mocking without `vi.mocked()` in Vitest tests
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T17:49:47.659Z
Learning: Applies to **/*.test.{ts,tsx} : Use 'vi.mock()' to mock external dependencies like file system and loggers in unit tests
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies in Vitest tests
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies in Vitest tests
Applied to files:
code/builders/builder-webpack5/src/presets/custom-webpack-preset.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases in Vitest tests
Applied to files:
code/builders/builder-webpack5/src/presets/custom-webpack-preset.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid direct function mocking without `vi.mocked()` in Vitest tests
Applied to files:
code/builders/builder-webpack5/src/presets/custom-webpack-preset.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking without the `spy: true` option in Vitest tests
Applied to files:
code/builders/builder-webpack5/src/presets/custom-webpack-preset.ts
⏰ 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: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/builders/builder-webpack5/src/presets/custom-webpack-preset.ts (1)
12-13: Document the intentional temporary disable with clear rationale; mismatch between PR title ("improve performance") and actual changes (complete disable via revert) requires clarification.The code currently disables the entire module-mocking workflow by commenting out all three components (loader, WebpackMockPlugin, and WebpackInjectMockerRuntimePlugin) in the webpack5 builder's custom preset. While the "Temporarily disabled" comments indicate this is intentional rather than accidental code, there's a significant mismatch:
- PR title states: "Improve performance of module-mocking plugins"
- Actual changes: Complete disable via revert (disables all plugin instantiation)
The plugin files themselves still exist with active code, and e2e tests for module-mocking still reference it, creating confusion about the intended end state. If this temporary disable is intentional:
- Add a detailed comment explaining why module-mocking is disabled (e.g., isolating a regression, feature rollback, testing baseline)
- Include a tracking issue or TODO linking to when/how this will be re-enabled
- Consider whether the PR title should reflect the actual changes ("Revert module-mocking integration" or similar)
If the plugins are meant to stay disabled, remove the orphaned code instead of leaving it commented.
This reverts commit 180015d.
valentinpalkovic
force-pushed
the
valentin/fix-hmr-for-webpack-users
branch
from
eae728e to
7b0a781
Compare
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts`:
- Around line 71-88: The code currently skips updates when mTimePreviewConfig <=
this.lastPreviewMtime which can miss rapid or restored changes; change the check
to strict equality (mTimePreviewConfig === this.lastPreviewMtime) and ensure
getPreviewConfigMtime returns a high-resolution timestamp (use fs.stat().mtimeMs
or equivalent) so mTimePreviewConfig and this.lastPreviewMtime are comparable
numeric mtimeMs values; keep the rest of the update flow (call to
extractAndResolveMocks, building mockMap, candidateSpecifiers, and assigning
this.lastPreviewMtime) the same, and apply the same equality-based check fix to
the other occurrence around the code referenced at lines 134-142.
| const mTimePreviewConfig = this.getPreviewConfigMtime(compiler); | ||
| if ( | ||
| this.lastPreviewMtime && | ||
| mTimePreviewConfig && | ||
| mTimePreviewConfig <= this.lastPreviewMtime | ||
| ) { | ||
| return; // unchanged | ||
| } | ||
| const resolved = this.extractAndResolveMocks(compiler); | ||
| this.mockMap = new Map( | ||
| this.extractAndResolveMocks(compiler).flatMap((mock) => [ | ||
| // first one, full path | ||
| resolved.flatMap((mock) => [ | ||
| [mock.absolutePath, mock], | ||
| // second one, without the extension | ||
| [mock.absolutePath.replace(/\.[^.]+$/, ''), mock], | ||
| ]) | ||
| ); | ||
| // divide by 2 because we add both the full path and the path without the extension | ||
| logger.info(`Mock map updated with ${this.mockMap.size / 2} mocks.`); | ||
| this.candidateSpecifiers = new Set(resolved.map((m) => m.path)); | ||
| this.lastPreviewMtime = mTimePreviewConfig; | ||
| logger.info(`Mock map updated with ${resolved.length} mocks.`); |
There was a problem hiding this comment.
Avoid skipping updates when mtime goes backward or has coarse resolution.
Using <= can miss changes (e.g., rapid edits within the same mtime tick or restored files). Prefer strict equality and use mtimeMs for higher resolution so mocks don’t go stale.
🔧 Proposed fix
- if (
- this.lastPreviewMtime &&
- mTimePreviewConfig &&
- mTimePreviewConfig <= this.lastPreviewMtime
- ) {
+ if (
+ this.lastPreviewMtime !== undefined &&
+ mTimePreviewConfig !== undefined &&
+ mTimePreviewConfig === this.lastPreviewMtime
+ ) {
return; // unchanged
}- const stat = fs.statSync?.(this.options.previewConfigPath);
- return stat?.mtime?.getTime?.();
+ const stat = fs.statSync?.(this.options.previewConfigPath);
+ return typeof stat?.mtimeMs === 'number'
+ ? stat.mtimeMs
+ : stat?.mtime?.getTime?.();Also applies to: 134-142
🤖 Prompt for AI Agents
In `@code/builders/builder-webpack5/src/plugins/webpack-mock-plugin.ts` around
lines 71 - 88, The code currently skips updates when mTimePreviewConfig <=
this.lastPreviewMtime which can miss rapid or restored changes; change the check
to strict equality (mTimePreviewConfig === this.lastPreviewMtime) and ensure
getPreviewConfigMtime returns a high-resolution timestamp (use fs.stat().mtimeMs
or equivalent) so mTimePreviewConfig and this.lastPreviewMtime are comparable
numeric mtimeMs values; keep the rest of the update flow (call to
extractAndResolveMocks, building mockMap, candidateSpecifiers, and assigning
this.lastPreviewMtime) the same, and apply the same equality-based check fix to
the other occurrence around the code referenced at lines 134-142.
github-project-automation
Bot
moved this from Empathy Queue (prioritized)
to Done
in Core Team Projects
…ck-users Webpack: Improve performance of module-mocking plugins (cherry picked from commit 20bf512)
github-actions
Bot
added
the
patch:done
2 participants
Closes #33778
What I did
During Hot Module Replacement (HMR) in development mode, the Webpack builder was performing unnecessary work on every compilation, leading to slower rebuild times. E.g.: The mocker runtime script was being regenerated and re-emitted on every compilation, even though it never changes and the mock map was being rebuilt on every watch run, even when the preview config file hadn't changed
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This pull request has been released as version
0.0.0-pr-33169-sha-8cabf98d. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-33169-sha-8cabf98d sandboxor in an existing project withnpx storybook@0.0.0-pr-33169-sha-8cabf98d upgrade.More information
0.0.0-pr-33169-sha-8cabf98dvalentin/fix-hmr-for-webpack-users8cabf98d1768996957)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=33169Summary by CodeRabbit
Bug Fixes
Performance
Reliability