Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add logic to sass rule conditional on sass-loader version #508

Merged
merged 2 commits into from
Aug 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions __mocks__/nonexistent/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"main": "./wat",
"version": "12.0.0"
}
4 changes: 4 additions & 0 deletions __mocks__/sass-loader/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"main": "./lib/api.js",
"version": "16.56.81"
}
3 changes: 2 additions & 1 deletion jest.config.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
module.exports = {
roots: ["<rootDir>/test"],
testPathIgnorePatterns: ["/__fixtures__/", "/__utils__/"]
testPathIgnorePatterns: ["/__fixtures__/", "/__utils__/"],
resolver: "<rootDir>/test/resolver"
}
14 changes: 8 additions & 6 deletions package/rules/sass.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,18 @@
/* eslint global-require: 0 */

const getStyleRule = require("../utils/getStyleRule")
const { canProcess } = require("../utils/helpers")
const { additional_paths: includePaths } = require("../config")
const { canProcess, packageMajorVersion } = require("../utils/helpers")
const { additional_paths: extraPaths } = require("../config")

module.exports = canProcess("sass-loader", (resolvedPath) =>
getStyleRule(/\.(scss|sass)(\.erb)?$/i, [
module.exports = canProcess("sass-loader", (resolvedPath) => {
const optionKey =
packageMajorVersion("sass-loader") > 15 ? "loadPaths" : "includePaths"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we 100% sure that this is the only change for modern vs. legacy support?

CC: @tomdracz @G-Rath

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return getStyleRule(/\.(scss|sass)(\.erb)?$/i, [
{
loader: resolvedPath,
options: {
sassOptions: { includePaths }
sassOptions: { [optionKey]: extraPaths }
}
}
])
)
})
10 changes: 9 additions & 1 deletion package/utils/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,18 @@ const loaderMatches = (configLoader, loaderToCheck, fn) => {
return fn()
}

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]
}
Comment on lines +44 to +49
Copy link

@coderabbitai coderabbitai bot Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Judahmeek any comment?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if there's no lock on the version in package.json (only yarn.lock)?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


module.exports = {
isBoolean,
ensureTrailingSlash,
canProcess,
moduleExists,
loaderMatches
loaderMatches,
packageMajorVersion
}
9 changes: 9 additions & 0 deletions test/package/development.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,15 @@ const { chdirTestApp, resetEnv } = require("../helpers")
const rootPath = process.cwd()
chdirTestApp()

jest.mock("../../package/utils/helpers", () => {
const original = jest.requireActual("../../package/utils/helpers")
const moduleExists = () => false
return {
...original,
moduleExists
}
})

describe("Development environment", () => {
beforeEach(() => jest.resetModules() && resetEnv())
afterAll(() => process.chdir(rootPath))
Expand Down
9 changes: 9 additions & 0 deletions test/package/environments/base.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,15 @@ chdirTestApp()
const baseConfig = require("../../../package/environments/base")
const config = require("../../../package/config")

jest.mock("../../../package/utils/helpers", () => {
const original = jest.requireActual("../../../package/utils/helpers")
const moduleExists = () => false
return {
...original,
moduleExists
}
})

describe("Base config", () => {
beforeEach(() => jest.resetModules() && resetEnv())
afterAll(() => process.chdir(rootPath))
Expand Down
9 changes: 9 additions & 0 deletions test/package/environments/development.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,15 @@ const { chdirTestApp, resetEnv } = require("../../helpers")
const rootPath = process.cwd()
chdirTestApp()

jest.mock("../../../package/utils/helpers", () => {
const original = jest.requireActual("../../../package/utils/helpers")
const moduleExists = () => false
return {
...original,
moduleExists
}
})

describe("Development specific config", () => {
beforeEach(() => {
jest.resetModules()
Expand Down
9 changes: 9 additions & 0 deletions test/package/environments/production.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,15 @@ const { chdirTestApp, resetEnv } = require("../../helpers")
const rootPath = process.cwd()
chdirTestApp()

jest.mock("../../../package/utils/helpers", () => {
const original = jest.requireActual("../../../package/utils/helpers")
const moduleExists = () => false
return {
...original,
moduleExists
}
})

describe("Production specific config", () => {
beforeEach(() => {
jest.resetModules()
Expand Down
11 changes: 11 additions & 0 deletions test/package/helpers.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
const { packageMajorVersion } = require("../../package/utils/helpers")

describe("packageMajorVersion", () => {
test("should find that sass-loader is v16", () => {
expect(packageMajorVersion("sass-loader")).toBe("16")
})

test("should find that nonexistent is v12", () => {
expect(packageMajorVersion("nonexistent")).toBe("12")
})
})
9 changes: 9 additions & 0 deletions test/package/index.test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
const index = require("../../package/index")

jest.mock("../../package/utils/helpers", () => {
const original = jest.requireActual("../../package/utils/helpers")
const moduleExists = () => false
return {
...original,
moduleExists
}
})

describe("index", () => {
test("exports webpack-merge v5 functions", () => {
expect(index.merge).toBeInstanceOf(Function)
Expand Down
9 changes: 9 additions & 0 deletions test/package/production.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,15 @@ const { chdirTestApp } = require("../helpers")
const rootPath = process.cwd()
chdirTestApp()

jest.mock("../../package/utils/helpers", () => {
const original = jest.requireActual("../../package/utils/helpers")
const moduleExists = () => false
return {
...original,
moduleExists
}
})

describe("Production environment", () => {
afterAll(() => process.chdir(rootPath))

Expand Down
9 changes: 9 additions & 0 deletions test/package/rules/index.test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
const rules = require("../../../package/rules/index")

jest.mock("../../../package/utils/helpers", () => {
const original = jest.requireActual("../../../package/utils/helpers")
const moduleExists = () => false
return {
...original,
moduleExists
}
})

describe("index", () => {
test("rule tests are regexes", () => {
rules.forEach((rule) => expect(rule.test instanceof RegExp).toBe(true))
Expand Down
23 changes: 23 additions & 0 deletions test/package/rules/sass.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
const sass = require("../../../package/rules/sass")

jest.mock("../../../package/utils/helpers", () => {
const original = jest.requireActual("../../../package/utils/helpers")
const canProcess = (rule, fn) => {
return fn("This path was mocked")
}
const packageMajorVersion = () => "15"
return {
...original,
canProcess,
packageMajorVersion
}
})

jest.mock("../../../package/utils/inliningCss", () => true)

describe("sass rule", () => {
test("contains loadPaths as the sassOptions key if sass-loader is v15 or earlier", () => {
expect(typeof sass.use[3].options.sassOptions.includePaths).toBe("object")
expect(typeof sass.use[3].options.sassOptions.loadPaths).toBe("undefined")
})
})
23 changes: 23 additions & 0 deletions test/package/rules/sass1.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
const sass = require("../../../package/rules/sass")

jest.mock("../../../package/utils/helpers", () => {
const original = jest.requireActual("../../../package/utils/helpers")
const canProcess = (rule, fn) => {
return fn("This path was mocked")
}
return {
...original,
canProcess
}
})

jest.mock("../../../package/utils/inliningCss", () => true)

describe("sass rule", () => {
test("contains loadPaths as the sassOptions key if sass-loader is v15 or earlier", () => {
expect(typeof sass.use[3].options.sassOptions.includePaths).toBe(
"undefined"
)
expect(typeof sass.use[3].options.sassOptions.loadPaths).toBe("object")
})
})
9 changes: 9 additions & 0 deletions test/package/staging.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,15 @@ const { chdirTestApp } = require("../helpers")
const rootPath = process.cwd()
chdirTestApp()

jest.mock("../../package/utils/helpers", () => {
const original = jest.requireActual("../../package/utils/helpers")
const moduleExists = () => false
return {
...original,
moduleExists
}
})

describe("Custom environment", () => {
afterAll(() => process.chdir(rootPath))

Expand Down
9 changes: 9 additions & 0 deletions test/package/test.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,15 @@ const { chdirTestApp } = require("../helpers")
const rootPath = process.cwd()
chdirTestApp()

jest.mock("../../package/utils/helpers", () => {
const original = jest.requireActual("../../package/utils/helpers")
const moduleExists = () => false
return {
...original,
moduleExists
}
})

describe("Test environment", () => {
afterAll(() => process.chdir(rootPath))

Expand Down
13 changes: 13 additions & 0 deletions test/resolver.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
const mapping = {
"css-loader": "this path was mocked",
"sass-loader/package.json": "../../__mocks__/sass-loader/package.json",
"nonexistent/package.json": "../../__mocks__/nonexistent/package.json"
}

function resolver(module, options) {
// If the path corresponds to a key in the mapping object, returns the fakely resolved path
// otherwise it calls the Jest's default resolver
return mapping[module] || options.defaultResolver(module, options)
}

module.exports = resolver