feat: test each postcss major version #37
Conversation
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds compatibility testing for multiple PostCSS major versions (7.x and 8.x) by implementing a hybrid plugin approach that automatically detects and adapts to the PostCSS version being used. The changes include test infrastructure, automated peer dependency management, and plugin code modifications to support both PostCSS 7 and 8 APIs.
- Implements a hybrid plugin pattern that detects PostCSS version and uses appropriate API
- Adds comprehensive test environments for PostCSS v7 and v8 with identical test suites
- Creates automated tooling to test versions and update peer dependencies based on test results
Reviewed Changes
Copilot reviewed 12 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/postcss-if-function/src/index.js | Modified plugin to use hybrid approach supporting both PostCSS 7 and 8 APIs |
| packages/postcss-if-function/scripts/update-peer-deps.js | Added script to automatically test versions and update peer dependencies |
| packages/postcss-if-function/package.json | Updated scripts, peer dependencies range, and added new dev dependency |
| packages/postcss-if-function/examples/postcss-version-8/* | Created PostCSS v8 test environment with comprehensive test suite |
| packages/postcss-if-function/examples/postcss-version-7/* | Created PostCSS v7 test environment with comprehensive test suite |
| packages/postcss-if-function/examples/README.md | Added documentation for version compatibility testing approach |
| packages/postcss-if-function/README.md | Updated compatibility documentation |
| packages/postcss-if-function/.npmignore | Excluded test and script directories from npm package |
Files not reviewed (2)
- packages/postcss-if-function/examples/postcss-version-7/package-lock.json: Language not supported
- packages/postcss-if-function/examples/postcss-version-8/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
packages/postcss-if-function/package.json:65
- [nitpick] The dependency name 'npm-run-all2' suggests this is a fork or alternative to 'npm-run-all'. Consider documenting why this specific package is used instead of the original 'npm-run-all' package, or use the original if there's no specific reason for the '2' variant.
"npm-run-all2": "^4.1.5",
| // In PostCSS 8, it's part of an object with other properties | ||
| const hybridPlugin = function (root, result) { | ||
| // This is the PostCSS 7 signature | ||
| if (result && result.processor !== undefined) { |
There was a problem hiding this comment.
The PostCSS version detection logic using result.processor !== undefined is fragile and may not reliably distinguish between PostCSS 7 and 8. Consider checking postcss().version or using a more robust detection method that doesn't rely on implementation details.
Copilot uses AI. Check for mistakes.
| // Test versions sequentially to avoid conflicts | ||
| for (const version of versions) { | ||
| const success = await testVersion(version.dir); // eslint-disable-line no-await-in-loop | ||
| if (success) { | ||
| testedVersions.push(version.range); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
[nitpick] Using await in a loop can be inefficient. Consider using Promise.all() or Promise.allSettled() to run version tests in parallel, which would be faster and more efficient.
| // Test versions sequentially to avoid conflicts | |
| for (const version of versions) { | |
| const success = await testVersion(version.dir); // eslint-disable-line no-await-in-loop | |
| if (success) { | |
| testedVersions.push(version.range); | |
| } | |
| } | |
| // Test versions in parallel for better efficiency | |
| const results = await Promise.all( | |
| versions.map(async (version) => { | |
| const success = await testVersion(version.dir); | |
| return success ? version.range : null; | |
| }) | |
| ); | |
| // Filter out unsuccessful tests | |
| testedVersions.push(...results.filter((range) => range !== null)); |
Copilot uses AI. Check for mistakes.
| const packageJson = JSON.parse(readFileSync(packageJsonPath, 'utf8')); | ||
|
|
||
| // Create a version range that includes all tested versions | ||
| const minVersion = testedVersions.sort()[0]; // Get the lowest version |
There was a problem hiding this comment.
Using sort() without a compare function performs lexicographical sorting, which may not work correctly for version ranges. For example, '^10.0.0' would sort before '^2.0.0'. Use a proper version comparison function or sort by extracted major version numbers.
Copilot uses AI. Check for mistakes.
Description
We should ensure compatibility with PostCSS versions. I'll limit it to version 7.x and 8.x based on that 6.x seems to be outdated (or we'll wait till someone requests this).
Type of Change
Testing
Checklist
Browser Testing
If applicable, please test in the following browsers:
Performance Impact
If this change affects performance:
Breaking Changes
If this introduces breaking changes, please describe:
Additional Notes
Any additional information that reviewers should know.