Add logic to sass rule conditional on sass-loader version #508
Conversation
WalkthroughThe changes introduced include the addition of Changes
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- mocks/nonexistent/package.json (1 hunks)
- mocks/sass-loader/package.json (1 hunks)
- jest.config.js (1 hunks)
- package/rules/sass.js (1 hunks)
- package/utils/helpers.js (1 hunks)
- test/package/helpers.test.js (1 hunks)
- test/package/rules/sass.test.js (1 hunks)
- test/package/rules/sass1.test.js (1 hunks)
- test/resolver.js (1 hunks)
Files skipped from review due to trivial changes (2)
- mocks/nonexistent/package.json
- mocks/sass-loader/package.json
Additional comments not posted (18)
jest.config.js (2)
3-3: LGTM!The
testPathIgnorePatternsconfiguration is correctly implemented.The code changes are approved.
4-4: LGTM!The
resolverproperty is correctly added to the Jest configuration.The code changes are approved.
test/package/helpers.test.js (2)
3-6: LGTM!The test case for
sass-loaderis correctly implemented and verifies the expected behavior.The code changes are approved.
8-10: LGTM!The test case for
nonexistentis correctly implemented and verifies the expected behavior.The code changes are approved.
test/resolver.js (1)
1-13: LGTM!The custom resolver is correctly implemented and provides the expected behavior.
The code changes are approved.
package/rules/sass.js (3)
4-5: LGTM!The imports and renaming of
additional_pathstoextraPathsare correctly implemented.The code changes are approved.
7-9: LGTM!The logic for determining the key for
sassOptionsbased on the version ofsass-loaderis correctly implemented.The code changes are approved.
14-14: LGTM!The configuration of
sassOptionswith the appropriate key is correctly implemented.The code changes are approved.
test/package/rules/sass1.test.js (2)
1-14: LGTM!The mocking of
canProcessandmoduleExistsfunctions is correctly implemented.The code changes are approved.
18-24: LGTM!The test correctly verifies the behavior of the
sassrule forsass-loaderversion 15 or earlier.The code changes are approved.
test/package/rules/sass.test.js (2)
1-16: LGTM!The mocking of
canProcess,moduleExists, andpackageMajorVersionfunctions is correctly implemented.The code changes are approved.
20-24: LGTM!The test correctly verifies the behavior of the
sassrule forsass-loaderversion 15 or earlier.The code changes are approved.
package/utils/helpers.js (6)
Line range hint
1-1: LGTM!The function is correctly implemented.
The code changes are approved.
Line range hint
3-3: LGTM!The function is correctly implemented.
The code changes are approved.
Line range hint
5-11: LGTM!The function is correctly implemented.
The code changes are approved.
Line range hint
13-13: LGTM!The function is correctly implemented.
The code changes are approved.
Line range hint
15-21: LGTM!The function is correctly implemented.
The code changes are approved.
Line range hint
23-41: LGTM!The function is correctly implemented.
The code changes are approved.
| const packageMajorVersion = (packageName) => { | ||
| // eslint-disable-next-line import/no-dynamic-require | ||
| const packageJsonPath = require.resolve(`${packageName}/package.json`) | ||
| // eslint-disable-next-line import/no-dynamic-require, global-require | ||
| return require(packageJsonPath).version.match(/^\d+/)[0] | ||
| } |
There was a problem hiding this comment.
Improve error handling.
The function does not handle cases where the package version is not in the expected format or where the package does not have a package.json file.
Apply this diff to improve error handling:
const packageMajorVersion = (packageName) => {
// eslint-disable-next-line import/no-dynamic-require
const packageJsonPath = require.resolve(`${packageName}/package.json`)
// eslint-disable-next-line import/no-dynamic-require, global-require
- return require(packageJsonPath).version.match(/^\d+/)[0]
+ const version = require(packageJsonPath).version
+ const match = version.match(/^\d+/)
+ if (!match) {
+ throw new Error(`Invalid version format for package ${packageName}`)
+ }
+ return match[0]
}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
What if there's no lock on the version in package.json (only yarn.lock)?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
The package.json that is checked is the one for the package in node_modules.
I forget the last time I saw a package in node_modules that didn't have a package.json.
There was a problem hiding this comment.
While technically not required by Node, the way the package managers and registries work it's all but guaranteed that there will be a package.json with a valid semantic version - the only way I would expect you could not have one would be if you manually (as in, cp, not npm install file://...) setup a dependency there in which case ... what are you doing anyway??
(among other things it's in a packages best interest to have a package.json because that's how you set the license, exports and dependencies)
fwiw you could do this using split('.')[0] instead of a regexp but I think they're probably pretty equivalent
| const packageMajorVersion = (packageName) => { | ||
| // eslint-disable-next-line import/no-dynamic-require | ||
| const packageJsonPath = require.resolve(`${packageName}/package.json`) | ||
| // eslint-disable-next-line import/no-dynamic-require, global-require | ||
| return require(packageJsonPath).version.match(/^\d+/)[0] | ||
| } |
| const packageMajorVersion = (packageName) => { | ||
| // eslint-disable-next-line import/no-dynamic-require | ||
| const packageJsonPath = require.resolve(`${packageName}/package.json`) | ||
| // eslint-disable-next-line import/no-dynamic-require, global-require | ||
| return require(packageJsonPath).version.match(/^\d+/)[0] | ||
| } |
There was a problem hiding this comment.
What if there's no lock on the version in package.json (only yarn.lock)?
| getStyleRule(/\.(scss|sass)(\.erb)?$/i, [ | ||
| module.exports = canProcess("sass-loader", (resolvedPath) => { | ||
| const optionKey = | ||
| packageMajorVersion("sass-loader") > 15 ? "loadPaths" : "includePaths" |
There was a problem hiding this comment.
It's seems to be the only relevant change: https://github.com/webpack-contrib/sass-loader/releases/tag/v16.0.0
Judahmeek
force-pushed
the
judahmeek/sass-options
branch
from
a764420 to
624c63b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- mocks/nonexistent/package.json (1 hunks)
- mocks/sass-loader/package.json (1 hunks)
- jest.config.js (1 hunks)
- package/rules/sass.js (1 hunks)
- package/utils/helpers.js (1 hunks)
- test/package/helpers.test.js (1 hunks)
- test/package/rules/sass.test.js (1 hunks)
- test/package/rules/sass1.test.js (1 hunks)
- test/resolver.js (1 hunks)
Files skipped from review due to trivial changes (2)
- mocks/nonexistent/package.json
- mocks/sass-loader/package.json
Files skipped from review as they are similar to previous changes (7)
- jest.config.js
- package/rules/sass.js
- package/utils/helpers.js
- test/package/helpers.test.js
- test/package/rules/sass.test.js
- test/package/rules/sass1.test.js
- test/resolver.js
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- test/package/development.test.js (1 hunks)
- test/package/environments/base.test.js (1 hunks)
- test/package/environments/development.test.js (1 hunks)
- test/package/environments/production.test.js (1 hunks)
- test/package/index.test.js (1 hunks)
- test/package/production.test.js (1 hunks)
- test/package/rules/index.test.js (1 hunks)
- test/package/rules/sass.test.js (1 hunks)
- test/package/rules/sass1.test.js (1 hunks)
- test/package/staging.test.js (1 hunks)
- test/package/test.test.js (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- test/package/rules/sass.test.js
Additional context used
GitHub Check: Linting
test/package/rules/index.test.js
[failure] 8-8:
Delete,test/package/rules/sass1.test.js
[failure] 10-10:
Delete,test/package/test.test.js
[failure] 12-12:
Delete,test/package/production.test.js
[failure] 12-12:
Delete,test/package/staging.test.js
[failure] 12-12:
Delete,test/package/development.test.js
[failure] 12-12:
Delete,test/package/index.test.js
[failure] 8-8:
Delete,test/package/environments/development.test.js
[failure] 11-11:
Delete,test/package/environments/production.test.js
[failure] 11-11:
Delete,test/package/environments/base.test.js
[failure] 18-18:
Delete,
Additional comments not posted (8)
test/package/rules/index.test.js (1)
3-10: LGTM! But remove the extra comma.The mock implementation is correctly set up. However, there is an extra comma in the returned object.
Apply this diff to remove the extra comma:
return { - ...original, - moduleExists, + ...original, + moduleExists }The code changes are approved.
Tools
GitHub Check: Linting
[failure] 8-8:
Delete,test/package/rules/sass1.test.js (2)
3-12: LGTM! But remove the extra comma.The mock implementation is correctly set up. However, there is an extra comma in the returned object.
Apply this diff to remove the extra comma:
return { - ...original, - canProcess, + ...original, + canProcess }The code changes are approved.
Tools
GitHub Check: Linting
[failure] 10-10:
Delete,
14-14: LGTM!The mock implementation is correctly set up.
The code changes are approved.
test/package/test.test.js (1)
7-14: LGTM! But remove the extra comma.The mock implementation is correctly set up. However, there is an extra comma in the returned object.
Apply this diff to remove the extra comma:
return { - ...original, - moduleExists, + ...original, + moduleExists }The code changes are approved.
Tools
GitHub Check: Linting
[failure] 12-12:
Delete,test/package/index.test.js (1)
3-10: Remove the extra comma in the return statement.The mock setup is correctly implemented, but there is an extra comma in the return statement.
Apply this diff to remove the extra comma:
return { ...original, moduleExists, - } + }The code changes are approved.
Tools
GitHub Check: Linting
[failure] 8-8:
Delete,test/package/environments/development.test.js (1)
6-13: Remove the extra comma in the return statement.The mock setup is correctly implemented, but there is an extra comma in the return statement.
Apply this diff to remove the extra comma:
return { ...original, moduleExists, - } + }The code changes are approved.
Tools
GitHub Check: Linting
[failure] 11-11:
Delete,test/package/environments/production.test.js (1)
6-13: Remove the extra comma in the return statement.The mock setup is correctly implemented, but there is an extra comma in the return statement.
Apply this diff to remove the extra comma:
return { ...original, moduleExists, - } + }The code changes are approved.
Tools
GitHub Check: Linting
[failure] 11-11:
Delete,test/package/environments/base.test.js (1)
13-20: LGTM! But remove the unnecessary comma.The mock implementation correctly isolates the tests from external dependencies. However, the comma at line 18 is unnecessary.
Apply this diff to remove the unnecessary comma:
jest.mock("../../../package/utils/helpers", () => { const original = jest.requireActual("../../../package/utils/helpers") const moduleExists = () => false return { ...original, - moduleExists, + moduleExists } })The code changes are approved.
Tools
GitHub Check: Linting
[failure] 18-18:
Delete,
test/package/production.test.js
Outdated
| jest.mock("../../../package/utils/helpers", () => { | ||
| const original = jest.requireActual("../../../package/utils/helpers") | ||
| const moduleExists = () => false | ||
| return { | ||
| ...original, | ||
| moduleExists, | ||
| } | ||
| }) |
There was a problem hiding this comment.
LGTM! But remove the trailing comma.
The mock implementation correctly overrides the moduleExists function. However, the trailing comma should be removed to adhere to linting rules.
The code changes are approved.
Apply this diff to remove the trailing comma:
- moduleExists,
+ moduleExistsCommittable 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.
| jest.mock("../../../package/utils/helpers", () => { | |
| const original = jest.requireActual("../../../package/utils/helpers") | |
| const moduleExists = () => false | |
| return { | |
| ...original, | |
| moduleExists, | |
| } | |
| }) | |
| jest.mock("../../../package/utils/helpers", () => { | |
| const original = jest.requireActual("../../../package/utils/helpers") | |
| const moduleExists = () => false | |
| return { | |
| ...original, | |
| moduleExists | |
| } | |
| }) |
Tools
GitHub Check: Linting
[failure] 12-12:
Delete,
test/package/staging.test.js
Outdated
| jest.mock("../../../package/utils/helpers", () => { | ||
| const original = jest.requireActual("../../../package/utils/helpers") | ||
| const moduleExists = () => false | ||
| return { | ||
| ...original, | ||
| moduleExists, | ||
| } | ||
| }) |
There was a problem hiding this comment.
LGTM! But remove the trailing comma.
The mock implementation correctly overrides the moduleExists function. However, the trailing comma should be removed to adhere to linting rules.
The code changes are approved.
Apply this diff to remove the trailing comma:
- moduleExists,
+ moduleExistsCommittable 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.
| jest.mock("../../../package/utils/helpers", () => { | |
| const original = jest.requireActual("../../../package/utils/helpers") | |
| const moduleExists = () => false | |
| return { | |
| ...original, | |
| moduleExists, | |
| } | |
| }) | |
| jest.mock("../../../package/utils/helpers", () => { | |
| const original = jest.requireActual("../../../package/utils/helpers") | |
| const moduleExists = () => false | |
| return { | |
| ...original, | |
| moduleExists | |
| } | |
| }) |
Tools
GitHub Check: Linting
[failure] 12-12:
Delete,
test/package/development.test.js
Outdated
| jest.mock("../../../package/utils/helpers", () => { | ||
| const original = jest.requireActual("../../../package/utils/helpers") | ||
| const moduleExists = () => false | ||
| return { | ||
| ...original, | ||
| moduleExists, | ||
| } | ||
| }) |
There was a problem hiding this comment.
LGTM! But remove the trailing comma.
The mock implementation correctly overrides the moduleExists function. However, the trailing comma should be removed to adhere to linting rules.
The code changes are approved.
Apply this diff to remove the trailing comma:
- moduleExists,
+ moduleExistsCommittable 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.
| jest.mock("../../../package/utils/helpers", () => { | |
| const original = jest.requireActual("../../../package/utils/helpers") | |
| const moduleExists = () => false | |
| return { | |
| ...original, | |
| moduleExists, | |
| } | |
| }) | |
| jest.mock("../../../package/utils/helpers", () => { | |
| const original = jest.requireActual("../../../package/utils/helpers") | |
| const moduleExists = () => false | |
| return { | |
| ...original, | |
| moduleExists | |
| } | |
| }) |
Tools
GitHub Check: Linting
[failure] 12-12:
Delete,
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- test/package/development.test.js (1 hunks)
- test/package/environments/base.test.js (1 hunks)
- test/package/environments/development.test.js (1 hunks)
- test/package/environments/production.test.js (1 hunks)
- test/package/index.test.js (1 hunks)
- test/package/production.test.js (1 hunks)
- test/package/rules/index.test.js (1 hunks)
- test/package/rules/sass1.test.js (1 hunks)
- test/package/staging.test.js (1 hunks)
- test/package/test.test.js (1 hunks)
Files skipped from review due to trivial changes (2)
- test/package/environments/base.test.js
- test/package/rules/sass1.test.js
Files skipped from review as they are similar to previous changes (8)
- test/package/development.test.js
- test/package/environments/development.test.js
- test/package/environments/production.test.js
- test/package/index.test.js
- test/package/production.test.js
- test/package/rules/index.test.js
- test/package/staging.test.js
- test/package/test.test.js
Judahmeek
force-pushed
the
judahmeek/sass-options
branch
from
ba7d6bc to
53d817f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- test/package/development.test.js (1 hunks)
- test/package/environments/base.test.js (1 hunks)
- test/package/environments/development.test.js (1 hunks)
- test/package/environments/production.test.js (1 hunks)
- test/package/index.test.js (1 hunks)
- test/package/production.test.js (1 hunks)
- test/package/rules/index.test.js (1 hunks)
- test/package/rules/sass.test.js (1 hunks)
- test/package/rules/sass1.test.js (1 hunks)
- test/package/staging.test.js (1 hunks)
- test/package/test.test.js (1 hunks)
Files skipped from review due to trivial changes (2)
- test/package/environments/base.test.js
- test/package/rules/sass1.test.js
Files skipped from review as they are similar to previous changes (9)
- test/package/development.test.js
- test/package/environments/development.test.js
- test/package/environments/production.test.js
- test/package/index.test.js
- test/package/production.test.js
- test/package/rules/index.test.js
- test/package/rules/sass.test.js
- test/package/staging.test.js
- test/package/test.test.js
Resolves #507
Pull Request checklist
Summary by CodeRabbit
New Features
package.jsonfiles for improved package management and module resolution.Bug Fixes
sass-loaderto ensure compatibility with different versions.Tests
packageMajorVersionfunction andsassrule configurations to ensure expected behaviors.moduleExistsin various test files to isolate test scenarios.