Conversation
✅ Deploy Preview for unocss ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Please clean up your code changes and stay focused on doing one thing at a time. |
- Hook into `finishModules` to scan modules when webpack cache is enabled - Manually read file content to bypass loader skipping on cache hits Fixes unocss#419
05842d9 to
d4aae6e
Compare
|
Apologies for the noise. I've cleaned up the unrelated changes/formatting and reverted the unnecessary files. The PR now only contains the fix for the MDX parser issue. |
|
commit: |
There was a problem hiding this comment.
Pull request overview
This PR addresses webpack persistent cache issues (#419) by ensuring that CSS classes are properly extracted from modules restored from cache. The fix adds a finishModules hook that manually reads and processes file content when webpack's persistent cache is enabled, preventing the loss of CSS extraction data between builds.
- Adds a new
finishModuleshook to handle modules restored from webpack's persistent cache - Implements file reading logic to extract CSS classes from cached modules
- Adds Buffer type import for type safety when handling file content
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,4 +1,5 @@ | |||
| import type { UserConfigDefaults } from '@unocss/core' | |||
| import type { Buffer } from 'node:buffer' | |||
There was a problem hiding this comment.
The Buffer type is a global type in Node.js and doesn't need to be explicitly imported from node:buffer. This import can be removed as Buffer is already available in the global scope.
| import type { Buffer } from 'node:buffer' |
| const content = await new Promise<string | Buffer | undefined>((resolve, reject) => { | ||
| compiler.inputFileSystem!.readFile(resource, (err, data) => { | ||
| if (err) | ||
| reject(err) | ||
| else | ||
| resolve(data) | ||
| }) | ||
| }) // let it throw if error |
There was a problem hiding this comment.
The error handling in this code will silently fail for file read errors. The comment on line 129 says "let it throw if error", but if a file read fails, the entire Promise.all on line 137 will reject, potentially causing the compilation to fail or behave unexpectedly. Consider wrapping the file read in a try-catch block and logging errors instead of letting them propagate, or at least handle them gracefully to avoid breaking the entire compilation process.
| const content = await new Promise<string | Buffer | undefined>((resolve, reject) => { | |
| compiler.inputFileSystem!.readFile(resource, (err, data) => { | |
| if (err) | |
| reject(err) | |
| else | |
| resolve(data) | |
| }) | |
| }) // let it throw if error | |
| let content: string | Buffer | undefined | |
| try { | |
| content = await new Promise<string | Buffer | undefined>((resolve, reject) => { | |
| compiler.inputFileSystem!.readFile(resource, (err, data) => { | |
| if (err) | |
| reject(err) | |
| else | |
| resolve(data) | |
| }) | |
| }) | |
| } | |
| catch (error) { | |
| compilation.warnings.push(new Error(`[@unocss/webpack] Failed to read file "${resource}": ${String(error)}`)) | |
| return | |
| } |
| if (compiler.options.cache) { | ||
| compilation.hooks.finishModules.tapPromise(PLUGIN_NAME, async (modules) => { | ||
| const { RESOLVED_ID_RE } = await ctx.getVMPRegexes() | ||
| const promises: Promise<any>[] = [] |
There was a problem hiding this comment.
The type Promise<any>[] loses type safety. Consider using Promise<void>[] instead since the promises are only used for awaiting and their resolved values are not used.
| const promises: Promise<any>[] = [] | |
| const promises: Promise<void>[] = [] |
| compilation.hooks.finishModules.tapPromise(PLUGIN_NAME, async (modules) => { | ||
| const { RESOLVED_ID_RE } = await ctx.getVMPRegexes() | ||
| const promises: Promise<any>[] = [] | ||
|
|
||
| for (const module of modules) { | ||
| const resource = (module as any).resource | ||
| if (resource && isCssId(resource)) | ||
| continue | ||
| if (resource && !RESOLVED_ID_RE.test(resource) && filter('', resource)) { | ||
| // For Webpack 5 persistent cache, we need to manually read the file content | ||
| // if the module is restored from cache, as loaders might be skipped. | ||
| promises.push( | ||
| (async () => { | ||
| if (!compiler.inputFileSystem) | ||
| return | ||
|
|
||
| const content = await new Promise<string | Buffer | undefined>((resolve, reject) => { | ||
| compiler.inputFileSystem!.readFile(resource, (err, data) => { | ||
| if (err) | ||
| reject(err) | ||
| else | ||
| resolve(data) | ||
| }) | ||
| }) // let it throw if error | ||
| if (content != null) | ||
| await ctx.extract(content.toString(), resource) |
There was a problem hiding this comment.
The finishModules hook processes all modules, including those that were just transformed. This means modules that went through the transform hook (lines 51-64) will have extract called twice - once during transform and once in finishModules. This duplicate processing could lead to incorrect CSS generation. Consider checking if a module was actually restored from cache before re-processing it, or track which modules have already been extracted to avoid duplicates.
| compilation.hooks.finishModules.tapPromise(PLUGIN_NAME, async (modules) => { | ||
| const { RESOLVED_ID_RE } = await ctx.getVMPRegexes() | ||
| const promises: Promise<any>[] = [] | ||
|
|
||
| for (const module of modules) { | ||
| const resource = (module as any).resource | ||
| if (resource && isCssId(resource)) | ||
| continue | ||
| if (resource && !RESOLVED_ID_RE.test(resource) && filter('', resource)) { | ||
| // For Webpack 5 persistent cache, we need to manually read the file content | ||
| // if the module is restored from cache, as loaders might be skipped. | ||
| promises.push( | ||
| (async () => { | ||
| if (!compiler.inputFileSystem) | ||
| return | ||
|
|
||
| const content = await new Promise<string | Buffer | undefined>((resolve, reject) => { | ||
| compiler.inputFileSystem!.readFile(resource, (err, data) => { | ||
| if (err) | ||
| reject(err) | ||
| else | ||
| resolve(data) | ||
| }) | ||
| }) // let it throw if error | ||
| if (content != null) | ||
| await ctx.extract(content.toString(), resource) | ||
| })(), | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| await Promise.all(promises) | ||
| }) |
There was a problem hiding this comment.
The new persistent cache handling logic lacks test coverage. Consider adding tests that verify the behavior across the three scenarios mentioned in the PR description: first build (no cache), second build (partial cache), and third build (full cache). This would ensure the cache handling works correctly and prevents regressions.
|
@Jack-sh1 I apologize for the long delay in submitting this pull request, as I am not familiar with webpack. I am willing to merge and release this pull request as an attempt. |
Should be tested with:
All builds should generate identical CSS with all classes extracted.
Fixes
Closes #419