chore: reduce generated code #266
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe PR consolidates SDK constants into a new centralized module ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The review involves verifying the new Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 @@
## main #266 +/- ##
==========================================
+ Coverage 88.88% 89.13% +0.24%
==========================================
Files 23 24 +1
Lines 1260 1288 +28
Branches 230 230
==========================================
+ Hits 1120 1148 +28
Misses 84 84
Partials 56 56 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
A question about the README and if that is or is not intended to be generated.
Also curious if it will be a breaking change with moving the constants? I assume so, and it shouldn't block any improvements like this, just want to confirm so we know to document when we release
rhamzeh
force-pushed
the
chore/reduce-generation
branch
2 times, most recently
from
b35df26 to
dc58191
Compare
rhamzeh
force-pushed
the
chore/reduce-generation
branch
from
dc58191 to
dd24321
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR reduces the amount of generated code in the JavaScript SDK by moving some files from auto-generation to manual maintenance. The key changes consolidate SDK constants into a centralized module and remove auto-generated headers from manually maintained utility and test files.
Key Changes:
- Created a new centralized
constants/index.tsfile containing all SDK constants - Removed auto-generated file headers from utility functions, tests, and core files
- Updated
.openapi-generator-ignoreto excludeconfiguration.ts,common.ts, andbase.tsfrom generation
Reviewed Changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
constants/index.ts |
New file centralizing all SDK constants previously scattered across files |
configuration.ts |
Refactored to import constants from centralized module instead of defining them locally |
common.ts |
Updated to use centralized constants and removed auto-generated header |
client.ts |
Updated to import client-specific constants from centralized module |
base.ts |
Removed auto-generated header comment block |
validation.ts |
Removed auto-generated header comment block |
utils/*.ts |
Removed auto-generated headers from all utility files |
tests/**/*.ts |
Removed auto-generated headers from all test files |
README.md |
Simplified contributing section to reference CONTRIBUTING.md |
CONTRIBUTING.md |
Updated to clarify the contribution process for mixed auto-generated and manual files |
.openapi-generator/FILES |
Reduced list of generated files to reflect new structure |
.openapi-generator-ignore |
Added exclusions for manually maintained files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CONTRIBUTING.md (1)
13-13: Doc inconsistency: ToC note conflicts with new PR guidance.TOC says “We are not accepting Pull Requests at this time!”, but below you invite PRs. Remove the TOC note for consistency.
-* [Submitting Pull Requests](#submitting-pull-requests) [Note: We are not accepting Pull Requests at this time!] +* [Submitting Pull Requests](#submitting-pull-requests)
🧹 Nitpick comments (7)
configuration.ts (2)
36-41: Explicit defaults in GetDefaultRetryParams — LGTM.Signature and defaults are clear; consider adding an explicit return type alias later for reuse.
183-185: Avoid magic number for max retries; use SdkConstants.Replace the literal 15 with SdkConstants.RetryMaxAllowedNumber to keep policy in one place and prevent drift.
- if (this.retryParams?.maxRetry && this.retryParams.maxRetry > 15) { + if (this.retryParams?.maxRetry && this.retryParams.maxRetry > SdkConstants.RetryMaxAllowedNumber) {common.ts (1)
147-155: Minor: consider using header-name constants.parseRetryAfterHeader could reference SdkConstants.RetryAfterHeaderName and support case-insensitive lookup to keep names centralized.
- const retryAfterHeader = headers["retry-after"] || headers["Retry-After"]; + const retryAfterHeader = headers[SdkConstants.RetryAfterHeaderName.toLowerCase()] + || headers[SdkConstants.RetryAfterHeaderName];CONTRIBUTING.md (1)
40-43: Autogenerated files note — LGTM, with one nit.Text is clear. Since some files no longer carry auto-generated headers and are now ignored by the generator, consider explicitly listing these (base.ts, common.ts, configuration.ts) as “hand-maintained” to avoid confusion.
constants/index.ts (3)
17-23: Avoid manual version drift.Hard-coding SdkVersion/UserAgent risks mismatches with package.json. Consider injecting the version at build time or importing from package.json (tree-shaken) to keep them in sync.
-const SdkVersion = "0.9.0"; -const UserAgent = "openfga-sdk js/0.9.0"; +// e.g., inject via build: define SdkVersion from process.env.npm_package_version +const SdkVersion = process.env.SDK_VERSION ?? "0.0.0-dev"; +const UserAgent = `openfga-sdk js/${SdkVersion}`;
49-55: Promote reuse of RetryMaxAllowedNumber.Now that this constant exists, ensure validation in configuration.ts uses it instead of a literal 15. See suggested diff there.
74-85: Optional: consolidate rate-limit header handling.Since RetryAfterHeaderName/RateLimitReset* are defined here, refactor common.ts to use them for lookups to keep naming in one place.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
.openapi-generator-ignore(1 hunks).openapi-generator/FILES(1 hunks)CONTRIBUTING.md(1 hunks)README.md(1 hunks)base.ts(0 hunks)client.ts(2 hunks)common.ts(5 hunks)configuration.ts(3 hunks)constants/index.ts(1 hunks)errors.ts(0 hunks)tests/client.test.ts(0 hunks)tests/headers.test.ts(0 hunks)tests/helpers/default-config.ts(0 hunks)tests/helpers/index.ts(0 hunks)tests/helpers/nocks.ts(0 hunks)tests/index.test.ts(0 hunks)tests/telemetry/attributes.test.ts(0 hunks)tests/telemetry/configuration.test.ts(0 hunks)tests/telemetry/counters.test.ts(0 hunks)tests/telemetry/histograms.test.ts(0 hunks)tests/telemetry/metrics.test.ts(0 hunks)tests/validation.test.ts(0 hunks)utils/assert-never.ts(0 hunks)utils/chunk-array.ts(0 hunks)utils/generate-random-id.ts(0 hunks)utils/index.ts(0 hunks)utils/set-header-if-not-set.ts(0 hunks)utils/set-not-enumerable-property.ts(0 hunks)validation.ts(0 hunks)
💤 Files with no reviewable changes (21)
- utils/generate-random-id.ts
- tests/telemetry/counters.test.ts
- utils/chunk-array.ts
- base.ts
- utils/assert-never.ts
- tests/headers.test.ts
- tests/helpers/default-config.ts
- errors.ts
- tests/helpers/index.ts
- utils/set-not-enumerable-property.ts
- tests/helpers/nocks.ts
- tests/index.test.ts
- tests/telemetry/configuration.test.ts
- tests/telemetry/attributes.test.ts
- tests/client.test.ts
- tests/telemetry/metrics.test.ts
- tests/telemetry/histograms.test.ts
- tests/validation.test.ts
- utils/set-header-if-not-set.ts
- utils/index.ts
- validation.ts
🧰 Additional context used
🧬 Code graph analysis (1)
configuration.ts (1)
index.ts (1)
GetDefaultRetryParams(19-19)
🪛 LanguageTool
CONTRIBUTING.md
[style] ~39-~39: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ...acker.** ### Submitting Pull Requests Feel free to submit a Pull Request against this repo...
(FEEL_FREE_TO_STYLE_ME)
🔇 Additional comments (14)
.openapi-generator-ignore (1)
2-4: Confirm generator ownership shift is complete.Ignoring configuration.ts, common.ts, and base.ts is fine; verify no generation step still relies on them being produced and that any “auto generated” headers were removed from these files to avoid confusion. Also update any contributor docs/scripts that mention these as generated.
client.ts (2)
50-50: Centralize defaults via SdkConstants — LGTM.Import aligns this module with the new constants hub.
92-95: Consistent defaults/header names — LGTM.Using SdkConstants for parallelism, batch size, and headers removes hard-coded drift.
configuration.ts (2)
3-14: Constants centralization — LGTM.Default retry params and user agent now sourced from SdkConstants; reduces drift.
66-66: sdkVersion via SdkConstants — LGTM.Removes hard-coded literals from this class.
common.ts (4)
23-23: DUMMY_BASE_URL via constants — LGTM.Keeps docs/tests aligned with a single domain source.
131-131: Clamp backoff to MaxBackoffTimeInSec — LGTM.Prevents runaway delays; matches policy in constants.
136-140: Retry delay bounds — LGTM.Validation now references DefaultMinWaitInMs and RetryHeaderMaxAllowableDurationInSec.
266-266: Cap sleep to RetryHeaderMaxAllowableDurationInSec — LGTM.Prevents excessively long sleeps from Retry-After.
CONTRIBUTING.md (2)
26-26: Issue flow update — LGTM.Pointing to js-sdk/issues first is clearer for contributors.
32-37: Security note — LGTM.Explicitly directing vuln reports away from public issues is good.
README.md (1)
833-834: Contributing section — LGTM.Single link reduces duplication with CONTRIBUTING.md.
constants/index.ts (1)
139-165: Frozen SdkConstants as single source of truth — LGTM.Clear grouping and default export; adoption across modules reduces literals and drift.
.openapi-generator/FILES (1)
15-15: LGTM! Addition aligns with PR objectives.The addition of
constants/index.tsto the generated files list is consistent with the PR's goal of consolidating SDK constants into a centralized module. The significantly reduced file count demonstrates successful reduction of generated code, with only essential artifacts remaining (Models, API, docs, Constants, License, and auxiliary files).
4 participants
Description
This is a PoC of the JS SDK with reduced generation from the generator. The things still generated are:
Generated from openfga/sdk-generator#638
via
What problem is being solved?
How is it being solved?
What changes are made to solve it?
References
Review Checklist
mainSummary by CodeRabbit
Chores
Documentation