Skip to content

fix(isISBN): accept empty options when checking both versions - #2882

Open
a3ylf wants to merge 2 commits into
validatorjs:masterfrom
a3ylf:fix/isbn-empty-options
Open

fix(isISBN): accept empty options when checking both versions#2882
a3ylf wants to merge 2 commits into
validatorjs:masterfrom
a3ylf:fix/isbn-empty-options

Conversation

@a3ylf

@a3ylf a3ylf commented Sep 7, 2026

Copy link
Copy Markdown

Passing an empty options object to isISBN rejects valid ISBN-10 and ISBN-13 values. For example, isISBN('340101319X', {}) returns false, while omitting the second argument returns true. The README says both versions are checked when no version is provided.

Read version from options objects before applying the legacy version-argument handling. Empty options and an undefined or null version now use the documented default. Explicit versions and the legacy numeric/string arguments retain their behavior.

Validation on Node.js 24.15.0:

  • The three new regression tests failed before the fix.
  • npm test passed: 327 tests, including builds and ESLint; 100% statement, function, and line coverage.
  • A compatibility case for top-level null options covers all six conditions on the options-handling line.
  • Checked the generated Node, browser, and minified browser exports for both default options and legacy versions.
  • git diff --check passed.

The README already describes the expected behavior, so it needs no change. This is an independently reproduced bug; I found no existing matching issue or pull request.

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable; existing documentation already covers this behavior)
  • Tests written (where applicable)
  • References provided in PR (where applicable; existing README contract)

@codecov

codecov Bot commented Sep 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (a79ff98) to head (797b78e).

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #2882   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          114       114           
  Lines         2599      2600    +1     
  Branches       658       658           
=========================================
+ Hits          2599      2600    +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nrps9909 nrps9909 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Validated exact head 797b78e858d6a156651e623eff15da73e4564b35 against base a79ff980ab14257e795332989e497bdff3218e87.

Applying the PR test file to unchanged base reproduces three failures (empty options, undefined version, null version); the top-level null compatibility control passes. Head full npm test passes 327 tests with generated builds and ESLint on Node 24.15.0. All 13 exposed upstream checks pass.

I also checked 1,406 ISBN-shaped/malformed strings across 16 option cases on each of source, Node, browser, and minified browser: 22,496 observations per form. The option-dispatch oracle is the unchanged base explicit ISBN-10/13 results (their OR for the documented default), so this isolates option handling rather than claiming a new ISBN grammar oracle. Base has 1,204 mismatches per form; head has zero. Explicit numeric/string versions, legacy calls, invalid explicit versions, formatting controls, and checksum mutations are included.

This resolves the documented missing-version default without changing the checked explicit-version behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants