Conversation
cjbarth
added
dependencies
📝 WalkthroughWalkthroughMigrates ESLint from a legacy JSON config to a flat Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #533 +/- ##
==========================================
+ Coverage 73.05% 77.84% +4.78%
==========================================
Files 9 9
Lines 902 1029 +127
Branches 239 270 +31
==========================================
+ Hits 659 801 +142
- Misses 143 185 +42
+ Partials 100 43 -57 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/signed-xml.ts (1)
1097-1107:⚠️ Potential issue | 🔴 CriticalNormalize resolver output in
addAllReferencesbefore validation/iteration.Line 1097 assumes
xpath.selectWithResolver(...)always returns an array, but line 950–955 already handles cases where it returns a single node. Both code paths must be consistent to avoid false "xpath not found" errors whenselectWithResolverreturns a single node instead of an array.Proposed fix
- const nodes = xpath.selectWithResolver(ref.xpath ?? "", doc, this.namespaceResolver); - - if (!utils.isArrayHasLength(nodes)) { + const selectedNodes = xpath.selectWithResolver(ref.xpath ?? "", doc, this.namespaceResolver); + const nodes = isDomNode.isArrayOfNodes(selectedNodes) + ? selectedNodes + : isDomNode.isNodeLike(selectedNodes) + ? [selectedNodes] + : []; + + if (!utils.isArrayHasLength(nodes)) { throw new Error( `the following xpath cannot be signed because it was not found: ${ref.xpath}`, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/signed-xml.ts` around lines 1097 - 1107, In addAllReferences, normalize the output of xpath.selectWithResolver(ref.xpath ?? "", doc, this.namespaceResolver) to always be an array before calling utils.isArrayHasLength and before iterating; detect if the resolver returned null/undefined or a single node and convert to [] or [node] respectively so the subsequent length check and for (const node of nodes) { isDomNode.assertIsElementNode(node); } behave consistently; update references to nodes so downstream logic expects an array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 47-70: The ESLint peer dependency conflict is caused by
eslint@^10.0.2 vs `@typescript-eslint/`* (typescript-eslint@8.56.1,
`@typescript-eslint/parser`, `@typescript-eslint/eslint-plugin`) which require
eslint ^8.57.0 || ^9.0.0; fix by either downgrading "eslint" to a 9.x range
(e.g., "^9.22.0") or upgrading all `@typescript-eslint` packages
(typescript-eslint, `@typescript-eslint/parser`, `@typescript-eslint/eslint-plugin`)
to versions that declare compatibility with ESLint 10.x, and then run npm
install to verify peers; also if the PR intends to adopt TypeScript 5.9, update
the "typescript" entry (currently "5.8") to "5.9" to match the stated objective.
---
Outside diff comments:
In `@src/signed-xml.ts`:
- Around line 1097-1107: In addAllReferences, normalize the output of
xpath.selectWithResolver(ref.xpath ?? "", doc, this.namespaceResolver) to always
be an array before calling utils.isArrayHasLength and before iterating; detect
if the resolver returned null/undefined or a single node and convert to [] or
[node] respectively so the subsequent length check and for (const node of nodes)
{ isDomNode.assertIsElementNode(node); } behave consistently; update references
to nodes so downstream logic expects an array.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d34734c-0af7-40ed-b569-bbc100313903
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
.eslintignore.eslintrc.jsonAGENTS.mdeslint.config.jspackage.jsonsrc/signature-algorithms.tssrc/signed-xml.tssrc/types.tssrc/utils.tstest/c14n-non-exclusive-unit-tests.spec.tstest/c14nWithComments-unit-tests.spec.tstest/canonicalization-unit-tests.spec.tstest/document-tests.spec.tstest/hmac-tests.spec.tstest/key-info-tests.spec.tstest/saml-response-tests.spec.tstest/signature-integration-tests.spec.tstest/signature-object-tests.spec.tstest/signature-unit-tests.spec.tstest/utils-tests.spec.tstest/wsfed-metadata-tests.spec.ts
💤 Files with no reviewable changes (2)
- src/types.ts
- .eslintrc.json
| "@eslint/js": "^10.0.1", | ||
| "@istanbuljs/nyc-config-typescript": "^1.0.2", | ||
| "@prettier/plugin-xml": "^3.2.2", | ||
| "@types/chai": "^4.3.11", | ||
| "@types/mocha": "^10.0.6", | ||
| "@types/node": "^16.18.69", | ||
| "@typescript-eslint/eslint-plugin": "^6.18.1", | ||
| "@typescript-eslint/parser": "^6.18.1", | ||
| "chai": "^4.3.10", | ||
| "@prettier/plugin-xml": "^3.4.2", | ||
| "@types/chai": "^4.3.20", | ||
| "@types/mocha": "^10.0.10", | ||
| "@types/node": "^20.19.35", | ||
| "@typescript-eslint/eslint-plugin": "^8.56.1", | ||
| "@typescript-eslint/parser": "^8.56.1", | ||
| "chai": "^4.5.0", | ||
| "choma": "^1.2.1", | ||
| "ejs": "^3.1.9", | ||
| "eslint": "^8.56.0", | ||
| "eslint-config-prettier": "^9.0.0", | ||
| "eslint-plugin-deprecation": "^2.0.0", | ||
| "ejs": "^4.0.1", | ||
| "eslint": "^10.0.2", | ||
| "eslint-config-prettier": "^10.1.8", | ||
| "globals": "^17.4.0", | ||
| "lcov": "^1.16.0", | ||
| "mocha": "^10.2.0", | ||
| "nyc": "^15.1.0", | ||
| "prettier": "^3.1.0", | ||
| "prettier-plugin-packagejson": "^2.4.6", | ||
| "release-it": "^16.3.0", | ||
| "mocha": "^11.7.5", | ||
| "nyc": "^18.0.0", | ||
| "prettier": "^3.8.1", | ||
| "prettier-plugin-packagejson": "^2.5.22", | ||
| "release-it": "^19.2.4", | ||
| "source-map-support": "^0.5.21", | ||
| "ts-node": "^10.9.1", | ||
| "typescript": "^5.3.2" | ||
| "ts-node": "^10.9.2", | ||
| "typescript": "5.8", | ||
| "typescript-eslint": "^8.56.1" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import json, urllib.request, urllib.parse
deps = {
"eslint": "10.0.2",
"@eslint/js": "10.0.1",
"@typescript-eslint/parser": "8.56.1",
"@typescript-eslint/eslint-plugin": "8.56.1",
"typescript-eslint": "8.56.1",
"typescript": "5.8",
"ejs": "4.0.1"
}
def fetch(pkg):
url = f"https://registry.npmjs.org/{urllib.parse.quote(pkg, safe='')}"
with urllib.request.urlopen(url) as r:
return json.load(r)
for pkg, ver in deps.items():
data = fetch(pkg)
exists = ver in data.get("versions", {})
print(f"{pkg}@{ver} exists: {exists}")
if exists and pkg in {
"@typescript-eslint/parser",
"@typescript-eslint/eslint-plugin",
"typescript-eslint"
}:
peer = data["versions"][ver].get("peerDependencies", {})
print(f" peerDependencies: {peer}")
PYRepository: node-saml/xml-crypto
Length of output: 2682
🌐 Web query:
@typescript-eslint/eslint-plugin 8.56.1 npm peer dependencies TypeScript
💡 Result:
For @typescript-eslint/eslint-plugin@8.56.1, the TypeScript peer dependency range is:
typescript:>=4.8.4 <6.0.0[1][2]
Sources: package metadata showing the peerDependencies for @typescript-eslint/eslint-plugin@8.56.1 [1], and the typescript-eslint “Dependency Versions” page (TypeScript supported range) [2].
🌐 Web query:
typescript-eslint 8.56.1 npm package peer dependencies
💡 Result:
For the typescript-eslint npm package (the meta-package that depends on @typescript-eslint/eslint-plugin, @typescript-eslint/parser, etc.), the peerDependencies are:
eslint:^8.57.0 || ^9.0.0typescript:>=4.8.4 <6.0.0[1]
These align with the TypeScript-ESLint project’s published supported ESLint version range. [2]
Sources:
[1] (app.unpkg.com)
[2] (typescript-eslint.io)
Citations:
- 1: https://app.unpkg.com/typescript-eslint%408.43.0/files/package.json
- 2: https://typescript-eslint.io/users/dependency-versions/?utm_source=openai
🌐 Web query:
TypeScript 5.8 5.9 npm releases versions
💡 Result:
TypeScript npm package (typescript) releases in the 5.8 and 5.9 lines (as reflected by the TypeScript repo tags / npm-dist versions) include:
-
5.8 pre-releases:
5.8.0-beta,5.8.1-rc(tagged asv5.8-beta,v5.8-rc) [1] -
5.8 stable:
5.8.2,5.8.3(tagsv5.8.2,v5.8.3) [1] -
5.9 pre-releases:
5.9.0-beta,5.9.1-rc(tagsv5.9-beta,v5.9-rc) [1] -
5.9 stable:
5.9.2,5.9.3(tagsv5.9.2,v5.9.3) [1][2]
Sources: GitHub tags list for microsoft/TypeScript [1] and ecosyste.ms npm registry mirror showing current latest release 5.9.3 [2].
[1] (github.com)
[2] (packages.ecosyste.ms)
Citations:
- 1: https://github.com/microsoft/TypeScript/tags
- 2: https://packages.ecosyste.ms/registries/npmjs.org/packages/typescript
Resolve the ESLint major version conflict: typescript-eslint@8.56.1 requires eslint ^8.57.0 || ^9.0.0, but package.json declares eslint@^10.0.2.
This incompatibility will cause npm peer dependency failures during install. Downgrade eslint to 9.x or upgrade typescript-eslint (and related @typescript-eslint/* packages) to a version compatible with ESLint 10.x.
Additionally, TypeScript is pinned to 5.8, which satisfies the peer dependency range but differs from any 5.9 adoption claim in the PR objective.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 47 - 70, The ESLint peer dependency conflict is
caused by eslint@^10.0.2 vs `@typescript-eslint/`* (typescript-eslint@8.56.1,
`@typescript-eslint/parser`, `@typescript-eslint/eslint-plugin`) which require
eslint ^8.57.0 || ^9.0.0; fix by either downgrading "eslint" to a 9.x range
(e.g., "^9.22.0") or upgrading all `@typescript-eslint` packages
(typescript-eslint, `@typescript-eslint/parser`, `@typescript-eslint/eslint-plugin`)
to versions that declare compatibility with ESLint 10.x, and then run npm
install to verify peers; also if the PR intends to adopt TypeScript 5.9, update
the "typescript" entry (currently "5.8") to "5.9" to match the stated objective.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
eslint.config.js (1)
21-24: Scope Mocha globals to test files only.
globals.mochais currently available in all**/*.tsfiles, which can mask accidental test-global usage in production sources.Proposed fix
globals: { ...globals.node, - ...globals.mocha, ...globals.es2020, Attr: "readonly", ChildNode: "readonly", @@ { files: ["test/**/*.ts"], + languageOptions: { + globals: { + ...globals.mocha, + }, + }, rules: { "@typescript-eslint/no-unused-expressions": "off", }, },Also applies to: 54-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eslint.config.js` around lines 21 - 24, The ESLint config is leaking Mocha globals into all source files because ...globals.mocha is spread into the top-level globals; remove ...globals.mocha from the top-level globals and instead add an overrides entry (using overrides: [{ files: ["**/*.test.ts","**/*.spec.ts","test/**"], globals: { ...globals.node, ...globals.es2020, ...globals.mocha } }]) so Mocha globals are only enabled for test files; update both places where ...globals.mocha is currently used (the top-level globals block and the second occurrence) to follow this pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@eslint.config.js`:
- Line 41: Replace the core ESLint rule "no-use-before-define" with the
TypeScript-aware "@typescript-eslint/no-use-before-define" in the config: remove
or disable "no-use-before-define" and add
"@typescript-eslint/no-use-before-define": "error" (or desired setting) so
TypeScript syntax is handled correctly; update any rule ordering or plugin
entries if needed to ensure "@typescript-eslint" rules are applied.
In `@src/signed-xml.ts`:
- Around line 601-606: The loadSignature(string) path currently assigns the
parsed Document returned by utils.parseXml(signatureNode) to this.signatureNode,
which breaks SignedInfo direct-children lookup; change the logic in
loadSignature to detect when utils.parseXml(...) returns a Document and assign
its documentElement (the root Element) to this.signatureNode instead of the
Document, then set this.signatureXml from that element's string form; apply the
same fix to the other occurrence referenced (the block around the later
assignment to this.signatureNode/this.signatureXml) so both branches store the
parsed Element rather than the whole Document.
---
Nitpick comments:
In `@eslint.config.js`:
- Around line 21-24: The ESLint config is leaking Mocha globals into all source
files because ...globals.mocha is spread into the top-level globals; remove
...globals.mocha from the top-level globals and instead add an overrides entry
(using overrides: [{ files: ["**/*.test.ts","**/*.spec.ts","test/**"], globals:
{ ...globals.node, ...globals.es2020, ...globals.mocha } }]) so Mocha globals
are only enabled for test files; update both places where ...globals.mocha is
currently used (the top-level globals block and the second occurrence) to follow
this pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a283ce54-ddac-40d3-8389-2744f6845ca9
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
eslint.config.jspackage.jsonsrc/c14n-canonicalization.tssrc/enveloped-signature.tssrc/exclusive-canonicalization.tssrc/hash-algorithms.tssrc/signature-algorithms.tssrc/signed-xml.tssrc/utils.tstest/c14n-non-exclusive-unit-tests.spec.tstest/c14nWithComments-unit-tests.spec.tstest/canonicalization-unit-tests.spec.tstest/document-tests.spec.tstest/hmac-tests.spec.tstest/key-info-tests.spec.tstest/saml-response-tests.spec.tstest/signature-integration-tests.spec.tstest/signature-object-tests.spec.tstest/signature-unit-tests.spec.tstest/utils-tests.spec.tstest/wsfed-metadata-tests.spec.ts
✅ Files skipped from review due to trivial changes (3)
- src/hash-algorithms.ts
- src/c14n-canonicalization.ts
- src/exclusive-canonicalization.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- test/document-tests.spec.ts
- test/c14nWithComments-unit-tests.spec.ts
- test/c14n-non-exclusive-unit-tests.spec.ts
- test/signature-unit-tests.spec.ts
- test/canonicalization-unit-tests.spec.ts
- src/utils.ts
- test/utils-tests.spec.ts
- test/key-info-tests.spec.ts
- test/hmac-tests.spec.ts
- package.json
| "no-prototype-builtins": "error", | ||
| "one-var": ["error", "never"], | ||
| "no-duplicate-imports": "error", | ||
| "no-use-before-define": "error", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "eslint.config.js" -type fRepository: node-saml/xml-crypto
Length of output: 82
🏁 Script executed:
cat -n eslint.config.jsRepository: node-saml/xml-crypto
Length of output: 2235
🏁 Script executed:
head -50 eslint.config.js | cat -nRepository: node-saml/xml-crypto
Length of output: 1979
🏁 Script executed:
wc -l eslint.config.js && head -62 eslint.config.js | cat -nRepository: node-saml/xml-crypto
Length of output: 2255
🌐 Web query:
no-use-before-define vs @typescript-eslint/no-use-before-define TypeScript
💡 Result:
What’s the difference?
no-use-before-define (ESLint core)
- The built-in ESLint rule that flags using identifiers before they’re declared (variables, functions, classes, etc.). (eslint.org)
@typescript-eslint/no-use-before-define (TypeScript-ESLint)
- A TypeScript-aware version that extends the ESLint core rule to better handle TypeScript syntax/semantics. (typescript-eslint.io)
Which should you use in TypeScript?
- In TypeScript projects, the common guidance is: turn off the ESLint core rule and turn on
@typescript-eslint/no-use-before-define, to avoid incorrect reports and ensure TS syntax is handled properly. (typescript-eslint.io)
Typical config
// .eslintrc.cjs / .eslintrc.js
module.exports = {
rules: {
"no-use-before-define": "off",
"@typescript-eslint/no-use-before-define": "error",
},
};Citations:
- 1: https://eslint.org/docs/latest/rules/no-use-before-define?utm_source=openai
- 2: https://typescript-eslint.io/rules/no-use-before-define/?utm_source=openai
- 3: https://typescript-eslint.io/rules/no-use-before-define/?utm_source=openai
- 4: https://typescript-eslint.io/rules/no-use-before-define/?utm_source=openai
Use the TypeScript-aware no-use-before-define rule.
The core ESLint rule cannot properly handle TypeScript syntax and semantics, causing incorrect or noisy lint failures. Replace it with @typescript-eslint/no-use-before-define.
Proposed fix
- "no-use-before-define": "error",
+ "no-use-before-define": "off",
+ "@typescript-eslint/no-use-before-define": "error",📝 Committable 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.
| "no-use-before-define": "error", | |
| "no-use-before-define": "off", | |
| "@typescript-eslint/no-use-before-define": "error", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@eslint.config.js` at line 41, Replace the core ESLint rule
"no-use-before-define" with the TypeScript-aware
"@typescript-eslint/no-use-before-define" in the config: remove or disable
"no-use-before-define" and add "@typescript-eslint/no-use-before-define":
"error" (or desired setting) so TypeScript syntax is handled correctly; update
any rule ordering or plugin entries if needed to ensure "@typescript-eslint"
rules are applied.
| const signatureNodeParsed = | ||
| typeof signatureNode === "string" ? utils.parseXml(signatureNode) : signatureNode; | ||
| this.signatureNode = signatureNodeParsed; | ||
|
|
||
| this.signatureXml = signatureNode.toString(); | ||
| this.signatureXml = signatureNodeParsed.toString(); | ||
|
|
There was a problem hiding this comment.
Fix loadSignature(string) by storing the parsed signature element, not the whole document.
When signatureNode is a string, utils.parseXml(signatureNode) produces a document. Assigning that directly to this.signatureNode can cause SignedInfo lookup to fail in the direct-children check path.
🐛 Proposed fix
- const signatureNodeParsed =
- typeof signatureNode === "string" ? utils.parseXml(signatureNode) : signatureNode;
+ const signatureNodeParsed =
+ typeof signatureNode === "string"
+ ? utils.parseXml(signatureNode).documentElement
+ : signatureNode;
this.signatureNode = signatureNodeParsed;
this.signatureXml = signatureNodeParsed.toString();Also applies to: 628-631
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/signed-xml.ts` around lines 601 - 606, The loadSignature(string) path
currently assigns the parsed Document returned by utils.parseXml(signatureNode)
to this.signatureNode, which breaks SignedInfo direct-children lookup; change
the logic in loadSignature to detect when utils.parseXml(...) returns a Document
and assign its documentElement (the root Element) to this.signatureNode instead
of the Document, then set this.signatureXml from that element's string form;
apply the same fix to the other occurrence referenced (the block around the
later assignment to this.signatureNode/this.signatureXml) so both branches store
the parsed Element rather than the whole Document.
1 participant
Update dependencies. This includes work to migrate to the new eslint configuration structure.
Node@20 is now the minimum supported version. Typescript@5.9 is also in use. We have not yet moved to ESM-only. That is planned for v8 of this library.
Summary by CodeRabbit
Chores
Documentation
Refactor
Tests