refactor(beasties-webpack-plugin): rewrite to typescript #213
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
WalkthroughMigrates the Beasties Webpack plugin from JavaScript to TypeScript with comprehensive type definitions. Adds build configuration, replaces utilities with typed implementations, refines HtmlWebpackPlugin integration and asset processing with explicit typing. Updates the base Beasties class signature and test infrastructure. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. 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: 1
🧹 Nitpick comments (7)
packages/beasties-webpack-plugin/src/util.ts (1)
3-3: Consider stronger typing for the callback parameter.The callback parameter uses
(...rest: any[]) => void, which bypasses type safety. Consider using a more specific type or generic constraint to maintain type information through the tap utility.For example:
-export function tap(inst: Compiler | Compilation, hook: string, pluginName: string, async: boolean, callback: (...rest: any[]) => void): void { +export function tap<T extends (...args: any[]) => any>(inst: Compiler | Compilation, hook: string, pluginName: string, async: boolean, callback: T): void {packages/beasties-webpack-plugin/test/index.test.ts (1)
204-209: Clarify the purpose of theafterAllhook.The
afterAllhook writes the same content asbeforeEach. If this is intended to restore the fixture to a known state after all tests complete, this is appropriate. However, if tests are expected to clean up after themselves, consider whetherafterEachwould be more suitable to prevent test interdependence.packages/beasties-webpack-plugin/src/index.ts (5)
69-71: Consider typing the silent method parameter consistently.The
silentmethod accepts a_: stringparameter but the parameter name uses an underscore to indicate it's unused. For consistency with the rest of the logger interface, consider naming it more descriptively.- silent(_: string): void { }, + silent(message: string): void { },
131-131: Strengthen type annotation for assets parameter.The inline comment
/* CompilationAssets */suggests uncertainty about the type. The type{ [id: string]: sources.Source }is correct for Webpack 5's compilation assets.Consider importing and using the proper type:
import type { Compilation, Compiler, OutputFileSystem, sources, Asset } from 'webpack' // Then use: (assets: Compilation['assets'], callback: (err?: null | Error) => void) => {
135-144: Consider using a more functional approach for finding HTML assets.The imperative loop could be replaced with
Object.keys(assets).find()for improved readability.- let htmlAssetName: string | undefined - for (const name in assets) { - if (name.match(/\.html$/)) { - htmlAssetName = name - break - } - } + const htmlAssetName = Object.keys(assets).find(name => /\.html$/.test(name))
237-237: Redundant array initialization.The
styleSheetsIncludedarray is initialised but could be defined withconstand proper typing.- const styleSheetsIncluded: string[] = []; + const styleSheetsIncluded: string[] = [](Remove semicolon for consistency with the rest of the file.)
283-288: Consider exporting BeastiesStyleElement for extensibility.The
BeastiesStyleElementinterface is currently internal. If users need to extend or override methods that use this type, they'll need access to it.Consider whether this interface should be exported from index.d.ts for advanced usage scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/beasties-webpack-plugin/package.jsonis excluded by!**/package.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (8)
packages/beasties-webpack-plugin/build.config.ts(1 hunks)packages/beasties-webpack-plugin/src/index.d.ts(1 hunks)packages/beasties-webpack-plugin/src/index.ts(11 hunks)packages/beasties-webpack-plugin/src/util.js(0 hunks)packages/beasties-webpack-plugin/src/util.ts(1 hunks)packages/beasties-webpack-plugin/test/helpers.ts(1 hunks)packages/beasties-webpack-plugin/test/index.test.ts(2 hunks)packages/beasties/src/index.d.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/beasties-webpack-plugin/src/util.js
🧰 Additional context used
🧬 Code graph analysis (3)
packages/beasties-webpack-plugin/src/index.d.ts (2)
packages/beasties-webpack-plugin/src/index.ts (1)
BeastiesWebpackPlugin(54-281)packages/beasties/src/index.d.ts (2)
Beasties(17-47)Options(49-69)
packages/beasties-webpack-plugin/test/index.test.ts (2)
packages/beasties/src/index.ts (1)
writeFile(78-93)packages/beasties-webpack-plugin/test/helpers.ts (2)
compileToHtml(81-102)readFile(39-41)
packages/beasties-webpack-plugin/src/index.ts (2)
packages/beasties/src/index.d.ts (1)
Options(49-69)packages/beasties-webpack-plugin/src/util.ts (1)
tap(3-13)
🔇 Additional comments (11)
packages/beasties-webpack-plugin/test/helpers.ts (1)
73-73: Verify that returning only the first error's details is sufficient.The change from
info.errors?.join('\n')toinfo.errors?.[0]?.detailsmeans only the first error's details are returned. If multiple errors occur during compilation, the subsequent errors will be lost.Confirm whether this behaviour is intentional and whether losing information about additional errors is acceptable for the test scenarios.
packages/beasties/src/index.d.ts (1)
38-38: LGTM! Signature extension aligns with plugin requirements.The addition of the
_style: Nodeparameter enables derived classes (such as BeastiesWebpackPlugin) to access style element information during CSS asset retrieval, supporting the enhanced asset tracking and inlining logic.packages/beasties-webpack-plugin/test/index.test.ts (1)
216-216: LGTM! Filesystem test correctly validates CSS processing.The test properly verifies that the CSS file is pruned to just a newline after processing, confirming the plugin's CSS inlining behaviour.
packages/beasties-webpack-plugin/src/index.d.ts (1)
1-63: LGTM! Well-structured TypeScript declarations.The declaration file properly extends Beasties, uses appropriate override keywords, includes comprehensive JSDoc, and exports using CommonJS style for backward compatibility.
packages/beasties-webpack-plugin/src/index.ts (6)
55-59: LGTM! Proper type declarations for class properties.The property declarations with
declarekeyword provide strong typing without runtime overhead, correctly typed for Webpack and Beasties integration.
83-85: LGTM! Robust HtmlWebpackPlugin detection.The check correctly identifies HtmlWebpackPlugin by constructor name, providing safe detection without requiring direct imports.
94-105: Ensure error handling in the process callback.The
handleHtmlPluginDatacallback correctly propagates errors via the callback parameter. The async/await pattern with.catch(callback)properly channels both synchronous and asynchronous errors.
164-210: LGTM! Comprehensive CSS asset retrieval with fallback.The method properly handles both Webpack assets and filesystem reads, with appropriate warning messages. The asset tracking via
style.$$assetandstyle.$$assetNameenables proper pruning downstream.
215-231: Verify asset deletion timing.The method calls
this.compilation.deleteAsset()during the inline threshold check. Ensure this deletion occurs at the appropriate stage of Webpack's compilation lifecycle and doesn't interfere with other plugins that might reference the same asset.
271-271: LGTM! Proper source map preservation during pruning.The use of
SourceMapSourceensures that source maps are maintained when pruning external stylesheets, which is essential for debugging production builds.packages/beasties-webpack-plugin/build.config.ts (1)
7-9: Disregard this review comment — the configuration is correct.The review mischaracterises what
respectExternal: falsedoes. Per rollup-plugin-dts documentation,respectExternal: false(the default) excludes node_modules typings and treats externals as truly external — it does not inline types. This is the recommended setting for library builds declaring external dependencies. Both packages are configured correctly.Likely an incorrect or invalid review comment.
| this.options.publicPath | ||
| // from html-webpack-plugin | ||
| = compiler.options.output.publicPath || typeof compiler.options.output.publicPath === 'function' | ||
| ? compilation.getAssetPath(compiler.options.output.publicPath!, compilation) | ||
| : compiler.options.output.publicPath! |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the publicPath logic for function types.
The condition on line 79 checks typeof compiler.options.output.publicPath === 'function' after the || operator, which means if publicPath is falsy, the function check won't prevent the assignment on line 82. This could result in undefined being assigned to this.options.publicPath.
🏁 Script executed:
#!/bin/bash
# Search for how publicPath is used elsewhere in webpack configurations
rg -n "output\.publicPath" --type=ts -C3 -g '!node_modules'Length of output: 2153
Fix the publicPath assignment logic to handle undefined and function types correctly.
The review comment is correct. The condition compiler.options.output.publicPath || typeof compiler.options.output.publicPath === 'function' evaluates to false when publicPath is falsy (undefined/null), causing the else branch to assign compiler.options.output.publicPath!, which results in undefined being assigned to this.options.publicPath.
The logic should check for function type first, or restructure to prevent undefined assignment:
typeof compiler.options.output.publicPath === 'function' || compiler.options.output.publicPath
? compilation.getAssetPath(compiler.options.output.publicPath!, compilation)
: compiler.options.output.publicPath || ''
This ensures functions are handled and undefined defaults to an empty string, matching the defensive pattern used downstream at line 171.
🤖 Prompt for AI Agents
In packages/beasties-webpack-plugin/src/index.ts around lines 77 to 81, the
publicPath assignment incorrectly treats falsy values before checking for
function types, letting undefined be assigned; change the condition to check for
a function first and ensure a safe default (e.g., empty string) when publicPath
is undefined so that functions are resolved via compilation.getAssetPath(...)
and non-function undefined values fall back to ''. Update the ternary to
evaluate typeof compiler.options.output.publicPath === 'function' ||
compiler.options.output.publicPath first, call compilation.getAssetPath(...)
when true, and otherwise assign a safe default (empty string) to
this.options.publicPath.
1 participant
Refactor the webpack plugin to typescript.