fix: adjusted experimental rollup plugins config #52
Conversation
BREAKING CHANGE: Refactored the plugin api away from before/after plugin name, to enforce pre/post and type build/dts where build is default.
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces significant enhancements to 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 (
|
|
Thank you for following the naming conventions! 🙏 |
commit: |
prisis
changed the title
![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
🧹 Outside diff range and nitpick comments (7)
packages/packem/README.md (3)
402-419: Plugin API documentation looks good, but could be more comprehensive.The new plugin API documentation clearly explains the enforce property and type options. The plugin resolution order is well documented.
Consider adding:
- Examples for "dts" type plugins
- Common use cases for "pre" vs "post" enforcement
420-435: Configuration example is clear and practical.The example effectively demonstrates the new plugin API usage with a real-world plugin.
Consider adding an example that shows:
{ enforce: "pre", plugin: optimizeLodashImports(), - // type: "build" -> default value + type: "dts", }
402-435: Add migration guide for breaking changes.Since this is a breaking change from the previous "before"/"after" naming convention to "pre"/"post", it would be helpful to include a migration guide section.
Consider adding:
- A section titled "Migrating from previous versions"
- A table showing the mapping between old and new plugin configurations
- Examples of how to update existing plugins to the new format
packages/packem/src/types.ts (1)
68-72: Consider documenting the newRollupPluginsstructureAdding inline documentation or comments explaining the new
RollupPluginsstructure can improve maintainability and help other developers understand the purpose of theenforceandtypeproperties.Apply this suggestion to add comments:
export type RollupPlugins = { + /** + * Specifies the plugin's enforcement phase. + * - "pre": Plugin will be applied before regular plugins. + * - "post": Plugin will be applied after regular plugins. + */ enforce?: "pre" | "post"; plugin: Plugin; + /** + * Specifies the type of the plugin. + * - "build": Plugin is applied during the build phase. + * - "dts": Plugin is applied during the declaration type script phase. + * @default "build" + */ type?: "build" | "dts"; }[];packages/packem/src/rollup/get-rollup-options.ts (3)
498-499: Simplify conditional checks using optional chainingTo improve readability, you can replace the chained logical conditions with optional chaining:
context.options.rollup.css?.loaders?.length > 0 &&This refactor makes the condition more concise without altering its logic.
🧰 Tools
🪛 Biome
[error] 498-499: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
518-519: Use optional chaining for cleaner conditional expressionsConsider simplifying the condition using optional chaining:
context.options.rollup.css?.loaders?.length > 0 &&This approach enhances code clarity by reducing redundancy.
🧰 Tools
🪛 Biome
[error] 518-519: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
795-796: Simplify conditional check using optional chainingYou can refactor the condition to use optional chaining for clarity:
context.options.rollup.license?.path &&This simplifies the check while maintaining the intended logic.
🧰 Tools
🪛 Biome
[error] 795-796: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/packem/README.md(1 hunks)packages/packem/src/rollup/get-rollup-options.ts(7 hunks)packages/packem/src/rollup/utils/append-plugins.ts(0 hunks)packages/packem/src/types.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/packem/src/rollup/utils/append-plugins.ts
🧰 Additional context used
🪛 Biome
packages/packem/src/rollup/get-rollup-options.ts
[error] 498-499: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 518-519: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 597-598: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 795-796: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (39)
packages/packem/src/types.ts (2)
68-72: Verify that all references to RollupPluginName are removed
The type RollupPluginName has been removed from the codebase. Ensure that all references to this type have been updated or removed to prevent type errors.
Run the following script to check for any remaining references to RollupPluginName:
✅ Verification successful
Since the rg command returned no results, indicating that there are no occurrences of RollupPluginName in the codebase, I can generate the final response.
All references to RollupPluginName have been successfully removed
The verification confirms that there are no remaining references to RollupPluginName in the codebase, indicating that the type removal was properly handled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining uses of 'RollupPluginName' in the codebase.
# Test: Find occurrences of 'RollupPluginName'.
rg 'RollupPluginName'
Length of output: 21
68-72:
Ensure all plugin configurations use the updated RollupPlugins structure
The RollupPlugins type has been updated to use an optional enforce property replacing the previous before and after properties. Please verify that all plugin configurations throughout the codebase have been updated to match this new structure, and there are no remaining references to before or after.
Run the following script to identify any remaining references to before or after in plugin configurations:
✅ Verification successful
Based on the search results, I can now provide a final response:
The codebase has been updated to use the new enforce property correctly
The search results confirm that:
- The new
RollupPluginstype withenforceproperty is properly defined inpackages/packem/src/types.ts - The README.md documents the new plugin ordering using
enforce: 'pre'andenforce: 'post' - The only reference to old
before/afterproperties is in a test file (packem-build-jit.test.ts) which appears to be testing legacy behavior
The test file should be updated to use the new enforce property, but this is not a critical issue as it's just test code and doesn't affect runtime behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining uses of 'before' or 'after' in plugin configurations.
# Test: Find occurrences of 'before' or 'after' properties in Rollup plugin definitions.
rg -A 3 -B 3 $'plugin:\\s*{[^}]*\\b(before|after)\\b[^}]*}'
Length of output: 191
Script:
#!/bin/bash
# Let's try a different approach to search for plugin configurations
# Search for files containing 'RollupPlugins' type references
echo "=== Files referencing RollupPlugins type ==="
rg -l "RollupPlugins"
# Search for plugin configurations with 'before' or 'after'
echo -e "\n=== Searching for 'before' or 'after' in plugin configurations ==="
rg -A 5 -B 5 "(?:before|after).*plugin.*:|plugin.*(?:before|after)"
# Search for actual plugin usage patterns
echo -e "\n=== Plugin configuration patterns ==="
rg -A 3 "enforce.*:|plugin.*:"
Length of output: 97317
packages/packem/src/rollup/get-rollup-options.ts (37)
21-21: Import of 'RollupPlugins' type approved
The addition of RollupPlugins to the imports ensures that the new type definitions are available for use in the file.
59-88: Function 'sortUserPlugins' correctly categorizes plugins
The new sortUserPlugins function effectively sorts plugins into pre, normal, and post categories based on their enforce property and the specified type. The logic accounts for undefined plugins and handles type-specific filtering appropriately.
375-376: Plugins sorted using 'sortUserPlugins' for 'build' type
Plugins are now sorted using the sortUserPlugins function for the "build" type, ensuring they are applied in the correct order during the build process.
378-378: 'baseRollupOptions' called with 'build' type
The baseRollupOptions function is correctly called with "build" as the type parameter, aligning with the updated function signature.
446-450: Adding essential plugins with caching for performance
Plugins such as resolveFileUrlPlugin and resolveTypescriptMjsCtsPlugin are added with caching mechanisms, which enhances build performance by avoiding redundant computations.
453-465: Conditional inclusion of 'replacePlugin' and 'aliasPlugin'
The code conditionally includes replacePlugin and aliasPlugin based on the Rollup configuration options. This allows for flexible plugin management tailored to the user's settings.
467-467: Including pre-plugins in the correct order
Pre-plugins are inserted into the plugins array, ensuring they are applied before other plugins during the build process.
469-469: Including 'nodeResolver' in plugins array
The nodeResolver plugin is appropriately added to the plugins array, facilitating module resolution.
471-475: Conditional addition of 'polyfillPlugin'
The polyfillPlugin is conditionally included when polyfillNode is enabled, with correct handling of source maps and user-defined options.
477-481: Inclusion of 'JSONPlugin' based on configuration
The JSONPlugin is included when configured in the Rollup options, enabling JSON file handling within the build.
482-484: Adding 'chunkSplitter' and optional 'wasmPlugin'
The chunkSplitter plugin is added to manage code splitting, and the wasmPlugin is conditionally included to support WebAssembly modules when enabled.
486-496: Including 'isolatedDeclarationsPlugin' for declaration files
The isolatedDeclarationsPlugin is conditionally included based on the declaration settings, ensuring proper generation of isolated TypeScript declaration files.
524-524: Inclusion of 'rawPlugin' with caching for performance
The rawPlugin is added with caching, improving performance when processing raw asset files.
526-528: Adding transformer plugin with proper configuration
The transformer plugin is included by invoking context.options.transformer with the appropriate configuration obtained from getTransformerConfig. Ensure the transformer is correctly set up in the context options.
530-537: Including 'preserveDirectivesPlugin' with caching
The preserveDirectivesPlugin is added with caching to maintain directive comments in the output, which is important for preserving certain JavaScript behaviors.
539-546: Conditional inclusion of 'shebangPlugin'
The shebangPlugin is added when the build entries are executable and shebang handling is enabled. This ensures that scripts retain their executable status after the build.
548-554: Including 'cjsInteropPlugin' for CommonJS interop
The cjsInteropPlugin is conditionally included to enhance interoperability between CommonJS and ES modules, based on the configuration settings.
556-557: Adding dynamic import handling plugins
The fixDynamicImportExtension and dynamicImportVarsPlugin are added when dynamic variable imports are enabled, ensuring proper handling of dynamic imports in the build.
568-575: Preserving dynamic imports when configured
A custom plugin is added to preserve dynamic imports when preserveDynamicImports is enabled, which may be necessary for certain runtime behaviors.
576-576: Inclusion of 'esmShimCjsSyntaxPlugin' for ES modules
The esmShimCjsSyntaxPlugin is conditionally added to shim CommonJS syntax in ES modules, aiding compatibility when shimming is enabled.
578-585: Adding 'jsxRemoveAttributes' plugin with caching
The jsxRemoveAttributes plugin is included with caching to remove specified attributes from JSX elements, which can be useful for optimizing the output.
587-587: Including post-plugins in the correct sequence
Post-plugins are appropriately added at the end of the plugins array, ensuring they are applied after other plugins.
589-636: Adding analysis and utility plugins conditionally
Plugins like metafilePlugin, copyPlugin, licensePlugin, prependDirectivePlugin, and node10CompatibilityPlugin are conditionally added based on the configuration, providing functionalities like bundle analysis, file copying, license management, directive prepending, and compatibility support.
🧰 Tools
🪛 Biome
[error] 597-598: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
685-686: Sorting plugins for 'dts' type using 'sortUserPlugins'
In the DTS (declaration types) Rollup options, plugins are sorted using sortUserPlugins for the "dts" type, ensuring correct plugin application order.
736-738: Including essential plugins in DTS build
Essential plugins like resolveFileUrlPlugin and resolveTypescriptMjsCtsPlugin are included with caching in the DTS Rollup configuration.
740-743: Inclusion of 'JSONPlugin' in DTS build
The JSONPlugin is conditionally included in the DTS build based on the configuration, ensuring JSON files are handled correctly during type declaration generation.
745-754: Ignoring non-supported file types in DTS build
A custom plugin is added to ignore non-supported file types in the DTS build, preventing potential build errors from unsupported imports.
756-757: Resolving TypeScript paths and root directories
Plugins for resolving TypeScript configuration paths and root directories are conditionally included, aiding in accurate module resolution during the DTS build.
759-763: Conditional inclusion of 'replacePlugin' in DTS build
The replacePlugin is conditionally included in the DTS build to handle string replacements as configured.
765-771: Including 'aliasPlugin' with custom resolver in DTS build
The aliasPlugin is included with a custom resolver, ensuring that module aliases are correctly resolved during the DTS build.
773-777: Including pre-plugins and node resolver in DTS plugins
Pre-plugins and the nodeResolver are added to the DTS build's plugins array, maintaining consistency with the build configuration.
779-779: Including normal plugins in DTS build
Normal plugins are appropriately included in the DTS build, ensuring that plugins without 'pre' or 'post' enforcement are applied correctly.
781-788: Adding DTS plugin and 'cjsInteropPlugin' conditionally
The DTS plugin is included using memoization to optimize performance. The cjsInteropPlugin is conditionally added based on the configuration, similar to the build process.
789-790: Including 'patchTypescriptTypesPlugin' with caching
The patchTypescriptTypesPlugin is added with caching when type patching is enabled, allowing for modifications to generated type declarations.
791-792: Removing shebangs in DTS build
The removeShebangPlugin is included to strip shebang lines from type declaration files, preventing syntax errors.
793-794: Including post-plugins in DTS build
Post-plugins are added at the end of the DTS build's plugins array, ensuring proper plugin sequencing.
227-227: Updated 'baseRollupOptions' function signature
The type parameter in baseRollupOptions has been updated to accept "build" or "dts" instead of "dependencies" or "dts". Ensure that all invocations of this function are updated to reflect the new parameter values and that any internal logic dependent on type is consistent with this change.
To confirm that all calls have been updated, you can run the following script:
✅ Verification successful
Function signature change verified successfully
All calls to baseRollupOptions have been updated correctly to use the new type parameter values:
- Using
"build"type in one call - Using
"dts"type in another call
No instances of the old "dependencies" type parameter were found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that 'baseRollupOptions' calls use the new 'type' values
# Test: Search for calls to 'baseRollupOptions' with the old 'dependencies' type
# Expected: No occurrences found
rg 'baseRollupOptions\([^\)]*,"dependencies"\)'
Length of output: 49
Script:
#!/bin/bash
# Let's search for all calls to baseRollupOptions to verify the type parameter
rg 'baseRollupOptions\(' -A 2
# Also search for the function definition and its usage context
ast-grep --pattern 'baseRollupOptions = $_'
Length of output: 560
prisis
changed the title
…em [1.4.1](https://github.com/visulima/packem/compare/@visulima/packem@1.4.0...@visulima/packem@1.4.1) (2024-11-04) ### Bug Fixes * adjusted experimental rollup plugins config ([#52](#52)) ([1573157](1573157))
Refactored the plugin api away from before/after plugin name, to enforce pre/post and type build/dts where build is default.
Summary by CodeRabbit
Release Notes
New Features
README.mdfor improved user guidance.Improvements
Visulima Packembundler, allowing for better organization and application order of plugins.Bug Fixes
These updates aim to enhance the user experience by providing clearer instructions and more flexible plugin management.