feat(validator): add tree-shakeable ESM export, fix tests, add a standalone validator with zero dependencies#518
Open
maxkoryukov wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR implements a build pipeline to compile JSON schemas into standalone Ajv validators and distribute them via package exports. The changes include vendored formats, build infrastructure using esbuild, package configuration updates, entrypoint re-exports, CI workflow integration, and comprehensive tests. ChangesBuild Pipeline and Package Exports
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/release.yml (2)
33-43:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPublish job does not build distributable artifacts before
npm publish(Line 40).
dist/is generated and gitignored, and this job runs on a fresh runner. Withoutnpm ci && npm run buildhere (or artifact transfer), published package can miss files required by exports.Suggested fix
npm: runs-on: ubuntu-latest needs: check steps: - name: Checkout uses: actions/checkout@master + - name: Install + run: npm ci + - name: Build + run: npm run build - name: Publish run: | echo "//registry.npmjs.org/:_authToken=${{ secrets.NPM_AUTH_TOKEN }}" > ~/.npmrc npm publish --access public🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 33 - 43, The Publish step in the "npm" job runs npm publish on a fresh runner without building the distributable, so the gitignored dist/ folder will be missing; modify the Publish step (the step named "Publish" under job "npm") to install dependencies and build before publishing—e.g., run "npm ci" and "npm run build" (or restore artifacts if you upload from a prior job) before the existing npm publish command—and ensure the build output expected by package.json exports is generated and present on the runner.
13-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove Node 18 from the release check matrix; package.json declares
engines.node: ">=20".The workflow matrix at lines 13 and 31 tests Node 18, which violates the declared minimum version requirement in package.json. Align the matrix with supported engines to ensure CI tests against compatible versions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml at line 13, Update the GitHub Actions release matrix to stop testing Node 18: find the matrix entry that sets "node: [18, 20]" (and any duplicate matrix entries with "node: 18") and remove 18 so the array matches package.json's engines (e.g., "node: [20]" or include only supported versions >=20); ensure both occurrences of the node matrix are updated so CI only runs against Node 20+..github/workflows/main.yml (1)
9-9:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBuild step will fail on Node 18 in the current CI matrix (Line 9).
The build script uses
import ... with { type: 'json' }syntax (line 8 of tools/build.mjs), which requires Node.js 18.20.0 or later. However, the CI matrix specifiesnode: [18, 20]without pinning a minimum minor version, so it may install Node 18.0–18.19.x where this syntax is unsupported. This conflicts withpackage.jsondeclaringengines: ">=20". Either remove Node 18 from the matrix to align with the declared minimum, or pin it to18.20.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/main.yml at line 9, CI matrix entry "node: [18, 20]" is unsafe because tools/build.mjs uses "import ... with { type: 'json' }" which requires Node >=18.20 and package.json declares "engines: >=20"; fix by either removing Node 18 from the matrix to match package.json (leave "node: [20]") or pin the minor to a compatible 18 release ("node: [18.20, 20]"), and update .github/workflows/main.yml accordingly so the matrix aligns with tools/build.mjs and package.json.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@index.mjs`:
- Around line 6-10: The validate function always signals failure because both
branches call callback(..., false); update the success branch to call
callback(_v.errors, true) when _v(resumeObj) returns truthy so callers receive
true on valid input; locate this in the validate function where _v, _v.errors
and callback are used and change only the second callback's boolean to true.
In `@job-schema.json`:
- Around line 2-4: The schema declares "$schema":
"http://json-schema.org/draft-04/schema#" but uses the draft-06 keyword "$id";
replace the "$id" property with "id" to be draft-04 compatible (i.e., change the
identifier key named "$id" to id in the top-level object so the schema's
identifier uses the draft-04 "id" keyword while keeping the same identifier
value).
In `@package.json`:
- Around line 12-14: The "./ajv" package subpath's require field points to a
missing file ("./dist/index.js"); update the package.json exports for the
"./ajv" entry so the require target matches the published CJS artifact (e.g.,
change require from "./dist/index.js" to "./dist/index.cjs") or ensure the
missing "./dist/index.js" is produced; update the "./ajv" exports block
accordingly so CJS consumers can resolve the subpath.
In `@tools/build.mjs`:
- Around line 47-55: The CJS build produced by the esbuild.build call is not
bundled (the bundle option is commented out), so the generated dist/index.cjs
contains imports that resolve to invalid relative paths at runtime; fix this by
enabling bundling for the esbuild.build invocation (set bundle: true in the
options passed to esbuild.build) so the CJS artifact is self-contained and no
runtime import paths like './dist/resume-validator.mjs' remain unresolved.
---
Outside diff comments:
In @.github/workflows/main.yml:
- Line 9: CI matrix entry "node: [18, 20]" is unsafe because tools/build.mjs
uses "import ... with { type: 'json' }" which requires Node >=18.20 and
package.json declares "engines: >=20"; fix by either removing Node 18 from the
matrix to match package.json (leave "node: [20]") or pin the minor to a
compatible 18 release ("node: [18.20, 20]"), and update
.github/workflows/main.yml accordingly so the matrix aligns with tools/build.mjs
and package.json.
In @.github/workflows/release.yml:
- Around line 33-43: The Publish step in the "npm" job runs npm publish on a
fresh runner without building the distributable, so the gitignored dist/ folder
will be missing; modify the Publish step (the step named "Publish" under job
"npm") to install dependencies and build before publishing—e.g., run "npm ci"
and "npm run build" (or restore artifacts if you upload from a prior job) before
the existing npm publish command—and ensure the build output expected by
package.json exports is generated and present on the runner.
- Line 13: Update the GitHub Actions release matrix to stop testing Node 18:
find the matrix entry that sets "node: [18, 20]" (and any duplicate matrix
entries with "node: 18") and remove 18 so the array matches package.json's
engines (e.g., "node: [20]" or include only supported versions >=20); ensure
both occurrences of the node matrix are updated so CI only runs against Node
20+.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 06595f75-2d9b-4259-ab2b-ededb76270d4
📒 Files selected for processing (8)
.github/workflows/main.yml.github/workflows/release.yml.gitignoreindex.mjsjob-schema.jsonpackage.jsontools/ajv-formats-micro.mjstools/build.mjs
CJS and ESM versions of standalone AJV validator for "resume schema" with 0 additional dependencies. in order to avoid conflicts - exported as `@jsonresume/schema/ajv` the old export and validator - without changes
`npm run build` is called: - during testing - before publishing
maxkoryukov
force-pushed
the
feat/standalone-validator-esm-cjs
branch
from
b1def46 to
0a75f35
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Around line 16-18: The package exports "./validate": "./validate.mjs" (and
indirectly via the "./all" re-export in index.mjs) but the package.json "files"
array does not include validate.mjs, causing consumers to fail to resolve it;
update the package.json "files" array to include "validate.mjs" (or
"./validate.mjs") so the exported entry "./validate" and the "./all" re-export
can be published and resolved correctly.
In `@tests/resume-awards.test.cjs`:
- Around line 31-34: The test file contains a focused test and a debug log:
change the test declaration in the 'awards[].title - invalid' case from
it.only(...) back to it(...) and remove the stray console.log(err) inside the
validate callback so the suite runs all tests and no debug output is committed;
locate the test by the string 'awards[].title - invalid' and the validate(...)
callback in tests/resume-awards.test.cjs to apply these edits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 502a5752-ec5c-476f-9e46-8c162709b4c5
📒 Files selected for processing (8)
index.mjspackage.jsontest/0-package.spec.cjstest/0-package.spec.mjstest/awards-esm.spec.mjstests/resume-awards.test.cjstools/build.mjsvalidate.mjs
✅ Files skipped from review due to trivial changes (2)
- test/0-package.spec.cjs
- test/0-package.spec.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/build.mjs
maxkoryukov
force-pushed
the
feat/standalone-validator-esm-cjs
branch
2 times, most recently
from
4a6d002 to
0290455
Compare
…xported in general: optimized and tested the "export"-design of the package - the new export (ESM/CJS) implemented with node-conditional export is tested (see 0-package.spec - cjs and mjs files) - fixed "coerceTypes" bug in AJV config - copied "awards.spec" and the new "validate" (from ESM) used in the copied test - 'index.mjs' converted to a barrel file, and 'validate.mjs' added - making the package "tree-shakeable" at least in the ESM mode (modern JS)
maxkoryukov
force-pushed
the
feat/standalone-validator-esm-cjs
branch
from
0290455 to
a830630
Compare
Author
|
@thomasdavis hello, sorry for mentioning directly... I would be glad to know, if changes in this PR align well with the purpose of the package. If yes - i would improve the PR/add more a few more like this thanks |
maxkoryukov
changed the title
validator with zero dependenciesvalidator with zero dependencies
maxkoryukov
changed the title
validator with zero dependenciesvalidator with zero dependencies
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Improve package export and add CJS/ESM versions of a standalone AJV validator for "resume schema" with 0 additional dependencies.
also, the new "export"-design of the package makes "tree shaking" easier with ESM, one only needs to use
and only the JSON schema will be imported, so the bundler won't include "validate"-functionality in the final bundle (at least from this package). Though, for the "validate.mjs" - the approach is still not ideal. I think another "export" will be required - for "precompiled and standalone" function, and another function that "import ajv" and "import ajv-formats" - so bundler and the programmer can decide how they want to bundle things together.
in order to avoid conflicts - exported as
@jsonresume/schema/allthe old export and validator - without changes
Summary by CodeRabbit
New Features
./all,./validate,./schemas/resume.json,./schemas/job.json) for improved modular imports.Chores
Tests
i've tested like this: