refactor(beasties): rewrite in typescript #16
Conversation
|
Warning Rate limit exceeded@danielroe has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces several changes across multiple files in the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/[email protected], npm/[email protected] |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (9)
package.json (1)
Remove
microbundleand update build system consistentlyThe verification confirms inconsistent build tooling across packages:
- Main
beastiespackage usesunbuildfor buildingbeasties-webpack-pluginstill usesmicrobundle- Root
package.jsonhasmicrobundleas a dependency but doesn't use it directlySuggested changes:
- Remove
microbundlefrom rootpackage.jsonsince it's not used directly- Consider migrating
beasties-webpack-pluginto useunbuildfor consistency with the main package🔗 Analysis chain
Line range hint
44-44: Verify build tool transition.The AI summary mentions replacing
microbundlewithunbuild, butmicrobundleis still listed in the dependencies. If the build system has been transitioned tounbuild, this dependency should be removed andunbuildshould be added.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if unbuild is used in any package.json files rg -l "unbuild" -g "package.json" # Check for any remaining microbundle usage in build scripts rg "microbundle" -g "package.json"Length of output: 326
Script:
#!/bin/bash # Check the build script in the root package.json to see if it's using microbundle or unbuild rg '"build":|"dev":|"watch":|"compile":' package.json -A 1 # Check the build script in beasties package to see if it's using unbuild cat packages/beasties/package.jsonLength of output: 1720
packages/beasties/src/util.ts (2)
Line range hint
22-48: Consider adding defensive error handling for console methods.While the implementation is good, it might be worth adding try-catch blocks to handle environments where console methods might be undefined or restricted.
Here's a safer implementation pattern:
export const defaultLogger = { trace(msg) { + try { // eslint-disable-next-line no-console console.trace(msg) + } catch { + // Fallback to console.log or silently fail + try { console.log('[TRACE]', msg) } catch {} + } }, // Apply similar pattern to other methods } satisfies Logger
Line range hint
51-63: Consider a more efficient logger creation approach.The current implementation using reduce works but creates intermediate objects. A more efficient approach would be to use Object.fromEntries.
Here's a more efficient implementation:
export function createLogger(logLevel: LogLevel) { const logLevelIdx = LOG_LEVELS.indexOf(logLevel) - return LOG_LEVELS.reduce((logger: Partial<Logger>, type, index) => { - if (index >= logLevelIdx) { - logger[type] = defaultLogger[type] - } - else { - logger[type] = defaultLogger.silent - } - return logger - }, {}) + return Object.fromEntries( + LOG_LEVELS.map(type => [ + type, + LOG_LEVELS.indexOf(type) >= logLevelIdx + ? defaultLogger[type] + : defaultLogger.silent + ]) + ) as Partial<Logger> }packages/beasties/src/types.ts (2)
97-97: Add documentation for reduceInlineStyles property.The
reduceInlineStylesproperty lacks JSDoc documentation explaining its purpose and default value.Add documentation similar to other properties:
/** * Reduce duplicate inline styles by combining identical rules _(default: `false`)_ */ reduceInlineStyles?: boolean
79-85: Clarify fonts property documentation.The documentation for the
fontsproperty could be clearer about its relationship withinlineFontsandpreloadFonts.Consider updating the documentation:
/** * Shorthand for setting both `inlineFonts` and `preloadFonts` properties * @example * fonts: true // equivalent to { inlineFonts: true, preloadFonts: true } * fonts: false // equivalent to { inlineFonts: false, preloadFonts: false } */ fonts?: booleanpackages/beasties/src/index.ts (4)
386-394: Ensure consistent handling ofkeyframesoptionsIn the
processStylemethod, thekeyframesoption is processed to determine thekeyframesMode. There might be scenarios where unexpected values could lead to inconsistent behavior. Consider adding validation or default cases to handle all possible values ofoptions.keyframes.For example:
let keyframesMode = options.keyframes ?? 'critical' // we also accept a boolean value for options.keyframes if (keyframesMode === true) keyframesMode = 'all' if (keyframesMode === false) keyframesMode = 'none' + if (!['all', 'none', 'critical'].includes(keyframesMode)) { + keyframesMode = 'critical' + }
Line range hint
487-522: Handle potential exceptions in selector processingWhen filtering selectors, exceptions are caught and added to
failedSelectors, but the original selector is modified before the exception occurs. To aid in debugging, consider logging the original selector that caused the exception.Apply this diff to improve error handling:
let originalSel = sel // Strip pseudo-elements and pseudo-classes... sel = sel .replace(/(?<!\\)::?[a-z-]+(?![a-z-(])/gi, '') // Additional replacements try { return beastiesContainer.exists(sel) } catch (e) { - failedSelectors.push(`${sel} -> ${(e as Error).message || (e as Error).toString()}`) + failedSelectors.push(`${originalSel} -> ${(e as Error).message || (e as Error).toString()}`) return false }
Line range hint
566-571: Review logging of failed selectors for potential information leakageThe warning message logs all failed selectors, which could potentially expose sensitive information. Consider limiting the amount of logged data or sanitizing the output to prevent any unintended information disclosure.
For example, you could limit the number of selectors logged:
if (failedSelectors.length !== 0) { + const displayedSelectors = failedSelectors.slice(0, 10) this.logger.warn?.( `${failedSelectors.length } rules skipped due to selector errors:\n ${displayedSelectors.join( '\n ', )}${failedSelectors.length > 10 ? '\n ...' : ''}`, ) }
Line range hint
687-696: Use consistent units informatSizefunctionIn the
formatSizefunction, the abbreviations array uses 'kB', 'MB', 'GB', which implies base 10 units (kilobytes, megabytes). However, the calculation uses base 1024, which is appropriate for kibibytes (KiB), mebibytes (MiB), etc. Consider updating the abbreviations or calculations for consistency.Option 1: Use base 1000 for calculations:
const roundedSize = size / 1000 ** indexOption 2: Update abbreviations to binary units:
- const abbreviations = ['bytes', 'kB', 'MB', 'GB'] + const abbreviations = ['bytes', 'KiB', 'MiB', 'GiB']
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
- package.json (1 hunks)
- packages/beasties/package.json (2 hunks)
- packages/beasties/src/css.ts (9 hunks)
- packages/beasties/src/dom.js (0 hunks)
- packages/beasties/src/dom.ts (1 hunks)
- packages/beasties/src/index.d.ts (1 hunks)
- packages/beasties/src/index.ts (24 hunks)
- packages/beasties/src/text.css (0 hunks)
- packages/beasties/src/types.ts (1 hunks)
- packages/beasties/src/util.ts (3 hunks)
💤 Files with no reviewable changes (2)
- packages/beasties/src/dom.js
- packages/beasties/src/text.css
✅ Files skipped from review due to trivial changes (1)
- packages/beasties/src/index.d.ts
🧰 Additional context used
🪛 Biome
packages/beasties/src/css.ts
[error] 89-89: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/beasties/src/dom.ts
[error] 212-212: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (20)
packages/beasties/package.json (5)
33-35: LGTM! Export configuration follows Node.js best practices.The changes to use
.cjsextension for CommonJS and conventionalindex.*naming are improvements.
38-39: LGTM! Main/module fields are properly configured.The fields are correctly aligned with the exports configuration and follow dual package hazard guidelines.
42-42: LGTM! Distribution files configuration is correct for a TypeScript package.Removing
srcfrom the files array is appropriate as TypeScript source files shouldn't be published.
51-51: LGTM! Dependencies are properly configured.The addition of
css-whataligns with the existingcss-selectdependency, andunbuildis a suitable replacement formicrobundlein a TypeScript project.Also applies to: 62-62
✅ Verification successful
LGTM! The
css-whatdependency is correctly added and properly usedThe codebase directly uses
css-whatinpackages/beasties/src/dom.tsfor parsing CSS selectors in thecachedQuerySelectorfunction throughselectorParser. This validates the addition ofcss-whatas a direct dependency, not just as a peer dependency ofcss-select.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify css-what usage in the codebase rg -l 'css-what' # Check if css-what is used directly or only through css-select ast-grep --pattern 'require("css-what")' || ast-grep --pattern 'from "css-what"'Length of output: 231
Script:
#!/bin/bash # Let's check the actual usage of css-what in dom.ts cat packages/beasties/src/dom.ts # Also check if css-what is used through css-select ast-grep --pattern 'from "css-select"' # And verify the imports in dom.ts specifically ast-grep --pattern 'from "css-what"' packages/beasties/src/dom.tsLength of output: 8773
45-45: Verify type definitions handling with unbuild.The manual copying of type definitions (
cp src/index.d.ts dist/index.d.ts) suggests unbuild might not be properly configured for TypeScript declaration files. Consider configuring unbuild to handle type definitions automatically.✅ Verification successful
Manual copy of type definitions is necessary in this case
The current setup is actually correct and necessary because:
- The project uses a handwritten
index.d.tsfile with detailed type definitions and documentation- The
tsconfig.jsonhas"noEmit": true, indicating type generation is not handled by TypeScript- Unbuild is being used primarily for bundling, while the types are maintained separately
The manual copy step is the right approach here since we're using a pre-written type definition file rather than relying on automatic type generation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if unbuild configuration exists and verify its TypeScript settings fd -e config.ts -e config.js -e json -g '*unbuild*' # Verify if there are other type definition files that need handling fd -e d.ts -t fLength of output: 231
Script:
#!/bin/bash # Check unbuild configuration in build.config.ts (common location) fd build.config.ts # Look for any unbuild configuration in package.json rg -l "unbuild" packages/beasties/package.json # Check the content of src/index.d.ts to understand what types need handling cat packages/beasties/src/index.d.ts # Check if there's a tsconfig.json that might affect type generation fd tsconfig.jsonLength of output: 2510
Script:
#!/bin/bash # Check tsconfig.json content for declaration settings cat tsconfig.json # Check if unbuild is configured in package.json rg -A 5 -B 5 "unbuild" packages/beasties/package.json # Look for any build output to see if types are generated fd -t f . packages/beasties/dist/Length of output: 1214
packages/beasties/src/util.ts (1)
5-20: Well-structured type definitions with good documentation!The type definitions are well-designed with:
- Immutable log levels using
as const- Clear interface documentation
- Consistent method signatures
packages/beasties/src/types.ts (1)
1-2: LGTM! Clean type imports and exports.The type imports and re-exports are well-structured, following TypeScript best practices for creating a public API.
Also applies to: 104-104
packages/beasties/src/css.ts (13)
17-20: Imports are well-defined and enhance type safetyThe addition of type imports from 'postcss' and 'postcss-media-query-parser' improves the code's readability and type safety.
28-28: Explicit parameter typing inparseStylesheetfunctionSpecifying the
stylesheetparameter type asstringin theparseStylesheetfunction enhances type safety and makes the function interface clearer.
32-39: Well-definedSerializeStylesheetOptionsinterfaceDefining the
SerializeStylesheetOptionsinterface provides clarity on the expected options for theserializeStylesheetfunction, improving maintainability.
Line range hint
45-70: Enhanced type annotations inserializeStylesheetfunctionThe
serializeStylesheetfunction now includes explicit types for its parameters, which enhances code clarity and reduces potential type-related errors.
70-70: Nullish coalescing and optional chaining used appropriatelyThe use of optional chaining (
node?.type) and checks for properties likenode.selectorsare correctly implemented to prevent runtime errors when properties may be undefined.
97-106: Type annotations inmarkOnlyfunction enhance clarityThe
markOnlyfunction's updated type annotations for its parameters and return type improve code readability and ensure correct usage.
133-137: Correct use of type guards inwalkStyleRulesfunctionThe function appropriately checks if
nodehasnodesproperty before proceeding, preventing potential runtime errors.
155-166: Proper handling of optional parameters inwalkStyleRulesWithReverseMirrorThe function correctly handles cases where
node2might beundefinedornull, ensuring robustness in recursive calls.
175-179: Effective use of type guard inhasNestedRulesfunctionThe
hasNestedRulesfunction correctly identifies nodes with nested rules, improving the reliability of rule processing.
187-199: GenericsplitFilterfunction improves reusabilityMaking
splitFiltera generic function enhances its versatility across different types, promoting code reuse.
Line range hint
235-254: Secure media query validation invalidateMediaTypeandvalidateMediaQueryThe functions perform necessary checks to validate media types and queries, enhancing security by preventing injection issues.
258-262: Compatibility handling formediaParserimportThe conditional handling of the
mediaParserimport ensures compatibility across different module systems, which is crucial for broader environment support.
279-286: Declaration merging correctly extendspostcssNode interfaceExtending the
Nodeinterface with additional properties via declaration merging is appropriate and allows for custom properties needed in the codebase.
package.json
Outdated
| "devDependencies": { | ||
| "@antfu/eslint-config": "3.8.0", | ||
| "@codspeed/vitest-plugin": "3.1.1", | ||
| "@types/node": "^22.8.1", |
There was a problem hiding this comment.
Update @types/node version to match supported Node.js version.
The added @types/node@^22.8.1 references Node.js 22 which isn't released yet. Since the engine requirement is "node": ">=18.13.0", you should use a compatible version of @types/node:
- "@types/node": "^22.8.1",
+ "@types/node": "^18.13.0",📝 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.
| "@types/node": "^22.8.1", | |
| "@types/node": "^18.13.0", |
| export function isSubpath(basePath: string, currentPath: string) { | ||
| return !path.relative(basePath, currentPath).startsWith('..') | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for path parameters.
The function should validate inputs to ensure they are valid path strings.
Here's a more robust implementation:
export function isSubpath(basePath: string, currentPath: string) {
+ if (typeof basePath !== 'string' || typeof currentPath !== 'string') {
+ throw new TypeError('Both basePath and currentPath must be strings')
+ }
+ if (!basePath || !currentPath) {
+ throw new Error('Paths cannot be empty')
+ }
return !path.relative(basePath, currentPath).startsWith('..')
}📝 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.
| export function isSubpath(basePath: string, currentPath: string) { | |
| return !path.relative(basePath, currentPath).startsWith('..') | |
| } | |
| export function isSubpath(basePath: string, currentPath: string) { | |
| if (typeof basePath !== 'string' || typeof currentPath !== 'string') { | |
| throw new TypeError('Both basePath and currentPath must be strings') | |
| } | |
| if (!basePath || !currentPath) { | |
| throw new Error('Paths cannot be empty') | |
| } | |
| return !path.relative(basePath, currentPath).startsWith('..') | |
| } |
| /** | ||
| * Controls which keyframes rules are inlined. | ||
| * | ||
| * - **"critical":** _(default)_ inline keyframes rules that are used by the critical CSS. | ||
| * - **"all":** Inline all keyframes rules. | ||
| * - **"none":** Remove all keyframes rules. | ||
| */ | ||
| export type KeyframeStrategy = 'critical' | 'all' | 'none' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety for keyframes option.
The keyframes option in the Options interface accepts KeyframeStrategy | boolean, but the documentation for KeyframeStrategy doesn't explain how boolean values are interpreted. This could lead to confusion.
Consider either:
- Document the boolean behavior in the
KeyframeStrategytype documentation - Or create a separate type that includes boolean values:
export type KeyframeStrategyOption = KeyframeStrategy | booleanAlso applies to: 88-88
| external?: boolean | ||
| /** | ||
| * Inline external stylesheets smaller than a given size _(default: `0`)_ | ||
| */ | ||
| inlineThreshold?: number | ||
| /** |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using more specific types for size thresholds.
The inlineThreshold and minimumExternalSize properties accept any number, but they likely represent sizes in bytes.
Consider adding a type to make this explicit:
type Bytes = number
/**
* Inline external stylesheets smaller than a given size in bytes _(default: `0`)_
*/
inlineThreshold?: Bytes| /** | ||
| * The mechanism to use for lazy-loading stylesheets. | ||
| * | ||
| * Note: <kbd>JS</kbd> indicates a strategy requiring JavaScript (falls back to `<noscript>` unless disabled). | ||
| * | ||
| * - **default:** Move stylesheet links to the end of the document and insert preload meta tags in their place. | ||
| * - **"body":** Move all external stylesheet links to the end of the document. | ||
| * - **"media":** Load stylesheets asynchronously by adding `media="not x"` and removing once loaded. <kbd>JS</kbd> | ||
| * - **"swap":** Convert stylesheet links to preloads that swap to `rel="stylesheet"` once loaded ([details](https://www.filamentgroup.com/lab/load-css-simpler/#the-code)). <kbd>JS</kbd> | ||
| * - **"swap-high":** Use `<link rel="alternate stylesheet preload">` and swap to `rel="stylesheet"` once loaded ([details](http://filamentgroup.github.io/loadCSS/test/new-high.html)). <kbd>JS</kbd> | ||
| * - **"js":** Inject an asynchronous CSS loader similar to [LoadCSS](https://github.com/filamentgroup/loadCSS) and use it to load stylesheets. <kbd>JS</kbd> | ||
| * - **"js-lazy":** Like `"js"`, but the stylesheet is disabled until fully loaded. | ||
| * - **false:** Disables adding preload tags. | ||
| */ | ||
| export type PreloadStrategy = 'body' | 'media' | 'swap' | 'swap-high' | 'js' | 'js-lazy' |
There was a problem hiding this comment.
Documentation and type definition mismatch.
The documentation mentions a "default" strategy that moves stylesheet links and inserts preload meta tags, but this option is not included in the PreloadStrategy type. Additionally, the type allows false as per line 65, but this isn't documented in the main type description.
Consider either:
- Adding the default strategy to the type:
-export type PreloadStrategy = 'body' | 'media' | 'swap' | 'swap-high' | 'js' | 'js-lazy'
+export type PreloadStrategy = 'default' | 'body' | 'media' | 'swap' | 'swap-high' | 'js' | 'js-lazy'- Or clarifying in the documentation that 'body' is the default strategy if that's the case.
Also, update the type documentation to mention that false is a valid value.
Committable suggestion was skipped due to low confidence.
| readFile(filename: string): string | Promise<string> { | ||
| const fs = this.fs |
There was a problem hiding this comment.
Adjust readFile method return type to Promise<string>
The readFile method is declared to return string | Promise<string>, but it always returns a Promise<string>. To ensure consistency and clarity, consider changing the return type to Promise<string>.
Apply this diff to update the return type:
- readFile(filename: string): string | Promise<string> {
+ readFile(filename: string): Promise<string> {
const fs = this.fs📝 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.
| readFile(filename: string): string | Promise<string> { | |
| const fs = this.fs | |
| readFile(filename: string): Promise<string> { | |
| const fs = this.fs |
| urlFilter: (url: string) => boolean | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove deprecated urlFilter property
The urlFilter property is maintained for backward compatibility, but there's a TODO comment indicating it should be removed. Since it's undocumented and slated for removal, consider removing it to clean up the codebase.
Apply this diff to remove the deprecated property:
- // TODO: remove (undocumented) support
- urlFilter: (url: string) => booleanAdditionally, remove the associated assignment in the constructor:
this.logger = this.options.logger || createLogger(this.options.logLevel)
}
+ // Remove deprecated `urlFilter` code
- // @ts-expect-error TODO: remove support
- this.urlFilter = this.options.filter
- if (this.urlFilter instanceof RegExp) {
- this.urlFilter = this.urlFilter.test.bind(this.urlFilter)
- }Committable suggestion was skipped due to low confidence.
| options || {}, | ||
| ) | ||
|
|
||
| options: Options & Required<Pick<Options, 'logLevel' | 'path' | 'publicPath' | 'reduceInlineStyles' | 'pruneSource' | 'additionalStylesheets'>> & { allowRules: Array<string | RegExp> } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the options type declaration for better readability
The options property has a complex type that can be hard to read and maintain. Consider defining a separate interface or type alias to simplify the type declaration and improve readability.
Apply this diff to extract the type into a separate interface:
+interface BeastiesOptions extends Options, Required<Pick<Options, 'logLevel' | 'path' | 'publicPath' | 'reduceInlineStyles' | 'pruneSource' | 'additionalStylesheets'>> {
+ allowRules: Array<string | RegExp>
+}
export default class Beasties {
- options: Options & Required<Pick<Options, 'logLevel' | 'path' | 'publicPath' | 'reduceInlineStyles' | 'pruneSource' | 'additionalStylesheets'>> & { allowRules: Array<string | RegExp> }
+ options: BeastiesOptionsCommittable suggestion was skipped due to low confidence.
| // eslint-disable-next-line regexp/no-super-linear-backtracking,regexp/no-misleading-capturing-group | ||
| src = (decl.value.match(/url\s*\(\s*(['"]?)(.+?)\1\s*\)/) || [])[2] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor regular expression to avoid disabling ESLint rules
The regular expression used in line 606 requires disabling multiple ESLint rules. Consider refactoring the regex to improve performance and maintainability, and to avoid the need to disable ESLint rules.
Apply this diff to update the regular expression:
// TODO: parse this properly and generate multiple preloads with type="font/woff2" etc
- // eslint-disable-next-line regexp/no-super-linear-backtracking,regexp/no-misleading-capturing-group
- src = (decl.value.match(/url\s*\(\s*(['"]?)(.+?)\1\s*\)/) || [])[2]
+ src = (decl.value.match(/url\(\s*(['"]?)([^'")]+)\1\s*\)/) || [])[2]This refactored regex prevents super-linear backtracking and avoids misleading capturing groups.
📝 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.
| // eslint-disable-next-line regexp/no-super-linear-backtracking,regexp/no-misleading-capturing-group | |
| src = (decl.value.match(/url\s*\(\s*(['"]?)(.+?)\1\s*\)/) || [])[2] | |
| // TODO: parse this properly and generate multiple preloads with type="font/woff2" etc | |
| src = (decl.value.match(/url\(\s*(['"]?)([^'")]+)\1\s*\)/) || [])[2] |
| return new Promise<string>((resolve, reject) => { | ||
| // eslint-disable-next-line node/prefer-global/buffer | ||
| const callback = (err: NodeJS.ErrnoException | null, data: string | Buffer) => { | ||
| if (err) | ||
| reject(err) | ||
| else resolve(data) | ||
| else resolve(data.toString()) | ||
| } | ||
| if (fs && fs.readFile) { | ||
| fs.readFile(filename, callback) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use fs.promises for cleaner asynchronous file reading
The readFile method uses callbacks wrapped in a Promise, which can be simplified. Utilize fs.promises.readFile for a cleaner and more modern asynchronous implementation.
Apply this diff to refactor the method:
readFile(filename: string): Promise<string> {
const fs = this.fs
- return new Promise<string>((resolve, reject) => {
- // eslint-disable-next-line node/prefer-global/buffer
- const callback = (err: NodeJS.ErrnoException | null, data: string | Buffer) => {
- if (err)
- reject(err)
- else resolve(data.toString())
- }
- if (fs && fs.readFile) {
- fs.readFile(filename, callback)
- }
- else {
- readFile(filename, 'utf-8', callback)
- }
- })
+ if (fs && fs.promises?.readFile) {
+ return fs.promises.readFile(filename, 'utf-8')
+ } else {
+ return readFile(filename, 'utf-8')
+ }
}📝 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.
| return new Promise<string>((resolve, reject) => { | |
| // eslint-disable-next-line node/prefer-global/buffer | |
| const callback = (err: NodeJS.ErrnoException | null, data: string | Buffer) => { | |
| if (err) | |
| reject(err) | |
| else resolve(data) | |
| else resolve(data.toString()) | |
| } | |
| if (fs && fs.readFile) { | |
| fs.readFile(filename, callback) | |
| return new Promise<string>((resolve, reject) => { | |
| // eslint-disable-next-line node/prefer-global/buffer | |
| const callback = (err: NodeJS.ErrnoException | null, data: string | Buffer) => { | |
| if (err) | |
| reject(err) | |
| else resolve(data.toString()) | |
| } | |
| if (fs && fs.promises?.readFile) { | |
| return fs.promises.readFile(filename, 'utf-8') | |
| } else { | |
| return readFile(filename, 'utf-8') | |
| } | |
| }) |
🧰 Tools
🪛 Biome
[error] 68-68: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/beasties/src/css.ts (2)
Line range hint
45-70: Security: Improve CSS injection protectionThe current check for
</style>in declaration values might not catch all potential CSS injection vectors. Consider using a more robust sanitization approach.Suggested improvement:
- if (node?.type === 'decl' && node.value.includes('</style>')) { + if (node?.type === 'decl' && ( + node.value.includes('</style>') || + node.value.includes('</STYLE>') || + node.value.includes('<script>') || + node.value.includes('<SCRIPT>') + )) { return }Consider using a dedicated CSS sanitization library for more comprehensive protection against CSS-based attacks.
Line range hint
235-277: Consider improving media query validation maintainabilityWhile the validation is thorough, consider these improvements:
- The media features list might need updates for newer CSS specs
- The validation logic could be more maintainable
Consider extracting the validation logic into a separate class:
class MediaQueryValidator { private static readonly MEDIA_TYPES = new Set(['all', 'print', 'screen', 'speech']); private static readonly MEDIA_KEYWORDS = new Set(['and', 'not', ',']); // Allow easier updates for new CSS specs private static readonly BASE_FEATURES = [ 'width', 'aspect-ratio', 'color', 'color-index', 'grid', 'height', 'monochrome', 'orientation', 'resolution', 'scan' ] as const; private static readonly MEDIA_FEATURES = new Set( MediaQueryValidator.BASE_FEATURES.flatMap( feature => [feature, `min-${feature}`, `max-${feature}`] ) ); static validateNode(node: Child | Root): boolean { return this.validateMediaType(node); } static validateQuery(query: string): boolean { // Current validation logic } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
- package.json (1 hunks)
- packages/beasties/package.json (2 hunks)
- packages/beasties/src/css.ts (9 hunks)
- packages/beasties/src/index.ts (25 hunks)
- vitest.config.mts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- packages/beasties/package.json
🧰 Additional context used
🪛 Biome
packages/beasties/src/css.ts
[error] 89-89: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (6)
vitest.config.mts (2)
1-1: LGTM! Clean import of Node.js URL utilities.The addition of
fileURLToPathis appropriate for handling file paths in an ESM context.
8-12: LGTM! Verify test execution with TypeScript files.The alias configuration correctly points to the source files, which is ideal for testing. This aligns well with the TypeScript rewrite objective.
Let's verify the TypeScript setup:
✅ Verification successful
TypeScript setup verified successfully - source files and entry point are properly configured
The verification confirms:
- Source directory exists at
packages/beasties/src/- Entry point is correctly set as TypeScript file at
packages/beasties/src/index.ts- No leftover JavaScript files in the source directory
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of TypeScript source files and their extensions # Test 1: Check if the source directory exists echo "Checking source directory..." fd --type d "src$" packages/beasties # Test 2: Check the entry point file extension (.ts or .tsx expected) echo "Checking entry point file..." fd "index\.(ts|tsx)$" packages/beasties/src # Test 3: Verify no leftover JavaScript files in source echo "Checking for leftover JS files..." fd "\.js$" packages/beasties/srcLength of output: 412
packages/beasties/src/css.ts (4)
17-38: LGTM! Well-structured type imports and interface definition.The type imports are properly organized and the
SerializeStylesheetOptionsinterface is well-documented.
175-182: LGTM! Well-implemented type guard.The
hasNestedRulestype guard effectively narrows the type while handling edge cases like @Keyframes rules.
186-199: LGTM! Well-structured generic implementation.The
splitFilterfunction makes good use of TypeScript generics and provides clear type safety.
279-286: LGTM! Well-structured type declarations.The module augmentation for postcss Node interface is properly implemented.
| if (styleSheetsIncluded.includes(cssFile)) { | ||
| return undefined | ||
| return [] | ||
| } | ||
| styleSheetsIncluded.push(cssFile) | ||
| const style = document.createElement('style') | ||
| style.$$external = true | ||
| return this.getCssAsset(cssFile, style).then(sheet => [sheet, style]) | ||
| return this.getCssAsset(cssFile, style).then(sheet => [sheet, style] as const) |
There was a problem hiding this comment.
Ensure consistent return types in 'additionalStylesheets' mapping function
The mapping function in this.options.additionalStylesheets.map returns inconsistent types: it returns an empty array in one branch and a Promise resolving to a tuple in the other. This inconsistency can cause runtime errors when iterating over sources.
Apply this diff to fix the issue by filtering duplicates before mapping:
+ const uniqueStylesheets = this.options.additionalStylesheets.filter((cssFile, index, self) => self.indexOf(cssFile) === index);
- const sources = await Promise.all(
- this.options.additionalStylesheets.map((cssFile) => {
- if (styleSheetsIncluded.includes(cssFile)) {
- return []
- }
- styleSheetsIncluded.push(cssFile)
+ const sources = await Promise.all(
+ uniqueStylesheets.map((cssFile) => {
const style = document.createElement('style')
style.$$external = true
return this.getCssAsset(cssFile, style).then(sheet => [sheet, style] as const)
}),
)This ensures that duplicates are handled before mapping, and the mapping function consistently returns Promises resolving to a tuple.
📝 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.
| if (styleSheetsIncluded.includes(cssFile)) { | |
| return undefined | |
| return [] | |
| } | |
| styleSheetsIncluded.push(cssFile) | |
| const style = document.createElement('style') | |
| style.$$external = true | |
| return this.getCssAsset(cssFile, style).then(sheet => [sheet, style]) | |
| return this.getCssAsset(cssFile, style).then(sheet => [sheet, style] as const) | |
| const uniqueStylesheets = this.options.additionalStylesheets.filter((cssFile, index, self) => self.indexOf(cssFile) === index); | |
| const sources = await Promise.all( | |
| uniqueStylesheets.map((cssFile) => { | |
| const style = document.createElement('style') | |
| style.$$external = true | |
| return this.getCssAsset(cssFile, style).then(sheet => [sheet, style] as const) | |
| }), | |
| ) |
CodSpeed Performance ReportMerging #16 will improve performances by ×2.3Comparing Summary
Benchmarks breakdown
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16 +/- ##
==========================================
+ Coverage 82.74% 83.46% +0.71%
==========================================
Files 6 7 +1
Lines 1420 1173 -247
Branches 242 268 +26
==========================================
- Hits 1175 979 -196
+ Misses 245 194 -51 ☔ View full report in Codecov by Sentry. |
this PR rewrites
beastiesin TypeScriptwhile it doesn't touch the underlying implementation it does fix some bugs (such as not respecting a
falsekeyframes option) and adds some additional checks for greater safetySummary by CodeRabbit
Release Notes
New Features
beastiespackage.Loggerinterface to standardize logging functionality.dom.tsfile.Enhancements
Beastiesclass and other utility functions.beastiespackage.Bug Fixes
Documentation
Beastiesclass.