Repository navigation
perf: pass a precompiled options check to getOptions - #746
alexander-akait wants to merge 1 commit into
Conversation
this.getOptions(schema, check) skips compiling the options schema when the check passes (webpack/webpack#22745). Older webpack versions ignore the second argument and validate as before. src/options.check.js is generated by `schema-utils precompile`; `npm run lint` fails when it is out of date with options.json. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QYPkbvwyRWt2SHgXDoYRdo
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
WalkthroughThe change adds a generated validator for loader options and passes it to Priority: ➖ Normal Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 5 files. (3 skipped: 3… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: passing a precompiled options check to getOptions for improved performance. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 5 files. (3 skipped: 3 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
🔧 ESLint
-
If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.
eslint.config.mjs
-
typescript-eslint does not support TS 7.0.
Please see https://devblogs.microsoft.com/typescript/announcing-typescript-7-0/#running-side-by-side-with-typescript-6.0 to run typescript-eslint using the TS 6 API.
See also typescript-eslint/typescript-eslint#10940 for tracking typescript-eslint's support for TS >=7.1Oops! Something went wrong! :(
ESLint: 9.39.5
Error: typescript-eslint does not support TS 7.0.
at Object. (/.eslint-tmp/node_modules/@typescript-eslint/eslint-plugin/dist/index.js:50:11)
at Module._compile (node:internal/modules/cjs/loader:1830:14)
at Object..js (node:internal/modules/cjs/loader:1961:10)
at Module.load (node:internal/modules/cjs/loader:1553:32)
at Module._load (node:internal/modules/cjs/loader:1355:12)
at wrapModuleLoad (node:internal/modules/cjs/loader:255:19)
at Module.require (node:internal/modules/cjs/loader:1576:12)
at require (node:internal/modules/helpers:153:16)
at /.eslint-tmp/node_modules/eslint-plugin-jest/lib/rules/unbound-method.js:13:28
at Object. (/.eslint-tmp/node_modules/eslint-plugin-jest/lib/rules/unbound-method.js:22:3)
jest.config.js
-
ESLint skipped: the matched ESLint configuration already failed (config-incompatibility).
package.json
-
ESLint skipped: the matched ESLint configuration already failed (config-incompatibility).
- 3 others
-
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![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 info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
5b761833-65ee-48a3-a985-e5a334d351f4
⛔ Files ignored due to path filters: 1
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing: 8
.cspell.json.prettierignoreeslint.config.mjsjest.config.jspackage.jsonsrc/index.jssrc/options.check.jstest/validate-options.test.js
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| */ | ||
| export default async function loader(content, sourceMap, meta) { | ||
| const options = this.getOptions(schema); | ||
| const options = this.getOptions(schema, check); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
python3 - <<'PY'
import io
import json
import re
import tarfile
import urllib.request
def get(url):
req = urllib.request.Request(url, headers={"User-Agent": "review-inspection"})
with urllib.request.urlopen(req, timeout=30) as r:
return r.read()
meta = json.loads(get("https://registry.npmjs.org/webpack"))
versions = [
v for v in meta["versions"]
if v.startswith("5.") and re.fullmatch(r"5\.\d+\.\d+", v)
]
versions.sort(key=lambda v: tuple(map(int, v.split("."))))
print(f"published webpack 5.x releases: {len(versions)}")
contradictions = []
missing = []
signatures = {}
for version in versions:
info = meta["versions"][version]
tarball = info["dist"]["tarball"]
try:
data = get(tarball)
with tarfile.open(fileobj=io.BytesIO(data), mode="r:gz") as tf:
candidates = [
n for n in tf.getnames()
if n.endswith("/lib/NormalModule.js") or n.endswith("/lib/module/NormalModule.js")
]
if not candidates:
missing.append((version, "NormalModule.js not found"))
continue
name = candidates[0]
text = tf.extractfile(name).read().decode("utf-8", "replace")
pos = text.find("getOptions")
excerpt = text[pos:pos+500] if pos >= 0 else "getOptions not found"
signatures[version] = " ".join(excerpt.split())
# Look for a parameter list that includes a second argument at the definition.
definitions = re.findall(r"getOptions\s*:\s*\(([^)]*)\)", text)
if not definitions:
definitions = re.findall(r"getOptions\s*:\s*([A-Za-z_$][\\w$]*)", text)
if any("," in d for d in definitions):
contradictions.append((version, definitions, name))
except Exception as e:
missing.append((version, type(e).__name__ + ": " + str(e)))
print("releases with a getOptions definition containing a second parameter:", len(contradictions))
for row in contradictions:
print("CONTRADICTS", row)
print("uninspected releases:", len(missing))
for row in missing[:20]:
print("MISSING", row)
for version in (versions[:3] + versions[len(versions)//2:len(versions)//2+1] + versions[-3:]):
print("SIGNATURE", version, signatures.get(version, "<unavailable>"))
print("\nREPOSITORY SOURCES")
for path, start, end in (("src/index.js", 1, 55), ("package.json", 88, 101)):
print(f"--- {path}:{start}-{end}")
lines = open(path, encoding="utf-8").read().splitlines()
for n in range(start, min(end, len(lines)) + 1):
print(f"{n}: {lines[n-1]}")
PYRepository: webpack/postcss-loader
Length of output: 496
🏁 Script executed:
python3 - <<'PY'
import concurrent.futures
import io
import json
import re
import tarfile
import urllib.request
def get(url):
req = urllib.request.Request(url, headers={"User-Agent": "review-inspection"})
with urllib.request.urlopen(req, timeout=15) as r:
return r.read()
meta = json.loads(get("https://registry.npmjs.org/webpack"))
versions = sorted(
[v for v in meta["versions"] if re.fullmatch(r"5\.\d+\.\d+", v)],
key=lambda v: tuple(map(int, v.split("."))),
)
def inspect(version):
try:
data = get(meta["versions"][version]["dist"]["tarball"])
with tarfile.open(fileobj=io.BytesIO(data), mode="r:gz") as tf:
names = [n for n in tf.getnames() if n.endswith("/lib/NormalModule.js") or n.endswith("/lib/module/NormalModule.js")]
if not names:
return version, "missing source", ""
text = tf.extractfile(names[0]).read().decode("utf-8", "replace")
matches = re.findall(r"getOptions\s*:\s*\(([^)]*)\)", text)
if not matches:
matches = re.findall(r"getOptions\s*:\s*([A-Za-z_$][A-Za-z0-9_$]*)", text)
return version, "second parameter" if any("," in m for m in matches) else "schema only", " | ".join(matches)
except Exception as exc:
return version, "error", type(exc).__name__ + ": " + str(exc)
results = []
with concurrent.futures.ThreadPoolExecutor(max_workers=24) as pool:
for result in pool.map(inspect, versions):
results.append(result)
print("release_count", len(versions))
for version, status, detail in results:
if status != "schema only":
print("EXCEPTION", version, status, detail)
print("schema_only_count", sum(status == "schema only" for _, status, _ in results))
print("missing_or_error_count", sum(status != "schema only" for _, status, _ in results))
for version in versions[:2] + versions[-2:]:
row = next(r for r in results if r[0] == version)
print("sample", row)
print("REPOSITORY")
for path, start, end in (("src/index.js", 30, 38), ("package.json", 92, 99)):
print(f"--- {path}:{start}-{end}")
lines = open(path, encoding="utf-8").read().splitlines()
for n in range(start, end + 1):
print(f"{n}: {lines[n-1]}")
PYRepository: webpack/postcss-loader
Length of output: 1090
🏁 Script executed:
python3 - <<'PY'
import concurrent.futures
import io
import json
import re
import tarfile
import urllib.request
def get(url):
req = urllib.request.Request(url, headers={"User-Agent": "review-inspection"})
with urllib.request.urlopen(req, timeout=15) as r:
return r.read()
meta = json.loads(get("https://registry.npmjs.org/webpack"))
versions = sorted(
[v for v in meta["versions"] if re.fullmatch(r"5\.\d+\.\d+", v)],
key=lambda v: tuple(map(int, v.split("."))),
)
def body_after_get_options(text):
pos = text.find("getOptions")
if pos < 0:
return None
brace = text.find("{", pos)
if brace < 0:
return None
depth = 0
quote = None
escaped = False
for i in range(brace, len(text)):
c = text[i]
if quote:
if escaped:
escaped = False
elif c == "\\":
escaped = True
elif c == quote:
quote = None
elif c in "'\"`":
quote = c
elif c == "{":
depth += 1
elif c == "}":
depth -= 1
if depth == 0:
return text[brace:i+1]
return None
def inspect(version):
try:
data = get(meta["versions"][version]["dist"]["tarball"])
with tarfile.open(fileobj=io.BytesIO(data), mode="r:gz") as tf:
names = [n for n in tf.getnames() if n.endswith("/lib/NormalModule.js") or n.endswith("/lib/module/NormalModule.js")]
text = tf.extractfile(names[0]).read().decode("utf-8", "replace")
body = body_after_get_options(text)
if body is None:
return version, "no-body", ""
suspicious = []
if re.search(r"\barguments\s*\[\s*1\s*\]", body):
suspicious.append("arguments[1]")
if re.search(r"\b(check|checker|compiled)\b", body, re.I):
suspicious.append("checker-term")
if not re.search(r"\bvalidate\b|\bschema-utils\b", body):
suspicious.append("no-validation-token")
return version, "ok" if not suspicious else " ".join(suspicious), " ".join(body[:220].split())
except Exception as exc:
return version, "error", type(exc).__name__ + ": " + str(exc)
with concurrent.futures.ThreadPoolExecutor(max_workers=24) as pool:
results = list(pool.map(inspect, versions))
print("release_count", len(results))
for row in results:
if row[1] != "ok":
print("EXCEPTION", row)
print("clean_getOptions_bodies", sum(status == "ok" for _, status, _ in results))
print("exception_count", sum(status != "ok" for _, status, _ in results))
PYRepository: webpack/postcss-loader
Length of output: 33118
The declared webpack range does not provide the claimed optimization.
All 221 published stable webpack 5.x releases allowed by ^5.0.0 define getOptions with only the schema parameter. No release consumes the second argument as a precompiled checker. Webpack therefore continues its normal schema validation, so this change does not improve performance for webpack consumers.
Remove or defer the checker-specific changes until webpack releases support for this argument.
There was a problem hiding this comment.
Intended: it lands ahead of webpack/webpack#22745. Released webpack ignores the second argument and validates as before, so nothing changes for current users. Once #22745 ships, the fast path applies without another postcss-loader release. A maintainer can also hold this until then.
Generated by Claude Code
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #746 +/- ##
=======================================
Coverage 88.23% 88.23%
=======================================
Files 3 3
Lines 357 357
Branches 118 118
=======================================
Hits 315 315
Misses 38 38
Partials 4 4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Summary
this.getOptions(schema)makes webpack loadschema-utilsand compile the options schema with ajv the first time the loader runs. webpack/webpack#22745 lets a loader pass a precompiled check as the second argument:this.getOptions(schema, check). When the check passes, the schema isn't compiled. Invalid options still get full validation, so the error messages stay the same.src/options.check.jsis generated fromsrc/options.jsonbyschema-utils precompile(schema-utils 4.6.0, a new devDependency). It's about 6 KB and requires nothing at runtime.npm run lintnow includeslint:schema, which fails when the check is out of date.npm run fixregenerates it.webpackpeer range doesn't change.Tests
test/validate-options.test.jsnow also asserts that the check and the fullschema-utilsvalidation agree on every valid and invalid value it tests. webpack skips the schema when the check passes, so they must never disagree.npm run lintand the full suite (132 passed, 4 skipped, 258 snapshots) pass locally. Coverage is unchanged at 87.24%.Generated by Claude Code
Summary by CodeRabbit