v0.10.0: fs-free environment-agnostic core + ksef-client-ts/node entry (GH#24)#31
Merged
Conversation
Unit A of GH#24: move fs-backed symbols out of the root barrel into a dedicated `ksef-client-ts/node` entry, keeping the core import fs-free.
- Drop FileOfflineInvoiceStorage, the xsd-validator block, and FileHwmStore from the sub-barrels so root `export *` no longer re-exports them.
- Add src/node.ts re-exporting exactly those symbols from their deep modules.
- package.json: sideEffects:false, `./node` export (import/require + types), version 0.9.1 → 0.10.0.
- tsup.config.ts: add src/node.ts as a library entry with its own dts.
Verified by Daedalus: build emits dist/node.{js,cjs,d.ts,d.cts}; root bundle has 0 of the moved symbols, /node resolves all; lint clean; unit 2141 passed; e2e 115 passed.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
- 24 unit tests covering all moved symbols exported from ksef-client-ts/node in both ESM (import) and CJS (require) modes - 8 additional tests asserting moved symbols are absent from the root barrel - Type-check fixture (tests/fixtures/node-types-check.ts) compiled via tsconfig.node-check.json to prove node.d.ts and node.d.cts are wired correctly - Lint script extended to include the node-check tsconfig pass Co-authored-by: Daedalus <daedalus@agents.flopbut.local> Co-authored-by: Paperclip <noreply@paperclip.ing>
Adds an fs-cleanliness guard asserting dist/index.js and dist/index.cjs contain no fs usage, with dist/node.js as a positive control. Wired into CI after build and into the vitest unit suite. Co-Authored-By: Paperclip <noreply@paperclip.ing>
…247) (#30) Unit D of GH#24: repoint docs to ksef-client-ts/node, add v0.10.0 migration guide, CHANGELOG, README/index cards. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 49 minutes and 12 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughVersion 0.10.0 isolates Node.js-specific exports ( Changesksef-client-ts v0.10.0: Node-only export isolation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested labels
🚥 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: 5
🤖 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 `@packages/ksef-client-ts/CHANGELOG.md`:
- Line 16: The CHANGELOG entry for "Asynchronous crypto modules" contains method
names and implementation details that should be removed. Replace the specific
references to `loginWithCertificate` and `loginWithPkcs12` methods and the
"dynamically import" implementation detail with a user-focused description that
emphasizes the key benefits: reduced bundle size and transparent behavior for
users. Keep the message brief and remove technical "how" details, focusing
instead on the user-facing impact and what they need to know.
- Around line 9-12: The CHANGELOG entry for the Node.js-specific features
section includes excessive implementation details (specific class names like
FileOfflineInvoiceStorage and FileHwmStore, function names like
validateAgainstXsd, and import paths). Refactor this entry to focus on the
user-facing impact by explaining in a single concise sentence that
Node.js-specific features have been moved to a separate entry point to improve
portability and reduce bundle size for web/edge environments, and direct users
to consult the migration guide for information about which symbols moved and how
to import them. Remove all references to specific symbol names, function names,
and import paths from the CHANGELOG entry itself.
In `@packages/ksef-client-ts/package.json`:
- Line 46: The "lint" script in package.json runs two tsc commands, but only the
first one includes the --noEmit flag. Add the --noEmit flag to the second tsc
command (the one that uses tsconfig.node-check.json) to ensure that the lint
task never writes any artifacts and maintains consistency across both
type-checking operations.
In `@packages/ksef-client-ts/scripts/check-fs-free-core.mjs`:
- Around line 11-12: The `nodeBundlePath` constant only validates the ESM bundle
(`dist/node.js`), but `package.json` also publishes a CommonJS bundle at
`dist/node.cjs`. Add a new constant to define the CJS bundle path (e.g.,
`nodeCJSBundlePath`) set to `dist/node.cjs`, and then update the validation
logic around lines 72-76 to check both the ESM bundle and the newly defined CJS
bundle path to ensure both artifacts are present and valid. This prevents broken
CJS bundles from passing the guard.
- Around line 33-37: The regex patterns for readFile, writeFile, and homedir in
the check-fs-free-core.mjs file are too broad and match standalone identifiers
unrelated to the fs module, causing false-positive failures. Refine these three
pattern definitions to be context-aware by binding them to fs import or require
statements, such as matching patterns like fs.readFile, fs.writeFile, and
fs.homedir, or by ensuring the patterns only trigger when these functions are
actually called in the context of fs module usage.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d9c04f05-653d-4032-91b0-d5248df9ea84
📒 Files selected for processing (21)
.github/workflows/ci.ymlpackages/ksef-client-ts/CHANGELOG.mdpackages/ksef-client-ts/README.mdpackages/ksef-client-ts/docs/.vitepress/config.tspackages/ksef-client-ts/docs/deno.mdpackages/ksef-client-ts/docs/index.mdpackages/ksef-client-ts/docs/migration-v0.10.mdpackages/ksef-client-ts/docs/offline-mode.mdpackages/ksef-client-ts/docs/workflows.mdpackages/ksef-client-ts/docs/xml-serialization.mdpackages/ksef-client-ts/package.jsonpackages/ksef-client-ts/scripts/check-fs-free-core.mjspackages/ksef-client-ts/src/node.tspackages/ksef-client-ts/src/offline/index.tspackages/ksef-client-ts/src/validation/index.tspackages/ksef-client-ts/src/workflows/index.tspackages/ksef-client-ts/tests/fixtures/node-types-check.tspackages/ksef-client-ts/tests/unit/node-subpath.test.tspackages/ksef-client-ts/tests/unit/utils/fs-free-core.test.tspackages/ksef-client-ts/tsconfig.node-check.jsonpackages/ksef-client-ts/tsup.config.ts
💤 Files with no reviewable changes (2)
- packages/ksef-client-ts/src/offline/index.ts
- packages/ksef-client-ts/src/validation/index.ts
…LO-240) The Unit C type-check fixture imports ksef-client-ts/node, which resolves via package exports to dist/node.d.ts. The unit-test job ran 'yarn lint' (type check) before 'yarn build', so dist was absent and tsc failed with TS2307 on all Node versions. Build now runs first. Co-Authored-By: Paperclip <noreply@paperclip.ing>
Per CodeRabbit PR review on #31: the 0.10.0 entry for the Node-only features listed class/function names and import paths, which the project's CHANGELOG convention forbids. Condense to one user-facing sentence and point to the v0.10 migration guide for the moved exports. Co-Authored-By: Claude Code <noreply@anthropic.com>
Per CodeRabbit PR review on #31: the entry named specific auth methods and described the dynamic-import mechanism. Reword to the user-facing benefit (smaller core bundle, on-demand signing modules) without method names or implementation detail. Co-Authored-By: Claude Code <noreply@anthropic.com>
Per CodeRabbit PR review on #31: the fs-cleanliness guard only inspected the ESM node bundle (dist/node.js), but package.json also publishes the node entry as CommonJS (dist/node.cjs). A broken CJS node bundle would have passed the guard. Check both ESM and CJS node bundles. Co-Authored-By: Claude Code <noreply@anthropic.com>
… lint Per local Codex review on #31. (1) The core still imports node:crypto/stream/zlib, so 'environment-agnostic / portable across browsers' overstated it; reword README, docs homepage, and migration guide to 'fs-free core' that runs on Node, Deno, and edge runtimes. (2) The combined lint ran a project type-check whose fixture resolves the built /node dist, so a clean checkout (or release.yml, which lints before building) could fail; split it into a separate check:node-types script and run it after Build in CI, leaving lint self-contained. Co-Authored-By: Claude Code <noreply@anthropic.com>
Document the two non-obvious failure modes when moving an import to ksef-client-ts/node: TypeScript needs moduleResolution node16+ (legacy node10 fails with TS2307 despite working at runtime), and CommonJS require silently yields undefined (fails later as a TypeError, not at require time). Co-Authored-By: Claude Code <noreply@anthropic.com>
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.
Consolidated PR for GH#24 / FLO-240 — make the
ksef-client-tscore environment-agnostic (Node/Deno/browser/edge) by moving all filesystem- and native-binding-dependent features behind a dedicatedksef-client-ts/nodesubpath export.What changed
ksef-client-ts/nodeentry pointfs/native-binding leaks/nodesubpath resolution tests (ESM + CJS + types)Breaking change (v0.10.0)
Node-only symbols are no longer exported from the main entry. Import them from
ksef-client-ts/nodeinstead:FileOfflineInvoiceStorage,FileHwmStorevalidateAgainstXsd,resolveXsdFor,FA_XSD_PATHS,PEF_XSD_PATHS,libxmljsAvailable,isMissingLibxmljsErrorCrypto signing modules (
loginWithCertificate/loginWithPkcs12) are now dynamically imported to keep the core bundle small — public API unchanged.Migration guide:
packages/ksef-client-ts/docs/migration-v0.10.mdLocal validation (integration branch)
yarn lint(tsc --noEmit) ✓yarn build✓ — emits dualindex+nodeentries (ESM/CJS/DTS)yarn test✓ — 2167 unit tests pass, includingnode-subpath(24) and fs-cleanliness guardNotes for review
Closes FLO-244, FLO-245, FLO-246, FLO-247.
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes v0.10.0
Breaking Changes
FileOfflineInvoiceStorage,FileHwmStore,validateAgainstXsd) are now imported fromksef-client-ts/node.New Features
Documentation
Quality & CI
ksef-client-ts/nodetype exports.