chore!: give each package's Constants class a package-specific name - #1677
Open
spydon wants to merge 2 commits into
Open
chore!: give each package's Constants class a package-specific name#1677spydon wants to merge 2 commits into
spydon wants to merge 2 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (32)
💤 Files with no reviewable changes (2)
📝 WalkthroughWalkthroughThe change replaces shared ChangesClient package constants
GoTrue constants
Realtime constants
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
spydon
changed the title
QuintinWillison
left a comment
QuintinWillison
left a comment
Member
There was a problem hiding this comment.
Needing to rename an internal type to fit a downstream, external system smells wrong to me. 🤔
This raises questions:
- Should the symbols/identifiers being enumerated in your manifest include the package they're in also, so each package (as is ok in Flutter/Dart) can have its own
Constantsinternal? - But, actually, should internal symbols/identifiers even be enumerated in a public-facing manifest?
spydon
added a commit
that referenced
this pull request
Aug 10, 2026
## Why Four different representations of an HTTP method coexisted across the packages: | Package | Representation | Values | Wire string | |---|---|---|---| | `functions_client` | public `enum HttpMethod` | get, post, put, delete, patch | `method.name.toUpperCase()`, open-coded at two call sites | | `postgrest` | public `enum HttpMethod` | get, **head**, post, put, patch, delete | `String get value => name.toUpperCase()` | | `gotrue` | `@internal enum RequestMethodType` | get, post, put, patch, delete | none, switches to `http.Client`'s `get` / `post` / … | | `storage_client` | bare `String` | `'GET'`, `'POST'`, `'PUT'`, `'DELETE'`, `'HEAD'` | the literal itself | The two public enums shared a name while disagreeing on contents, which forced `supabase.dart` to export postgrest with `hide HttpMethod`, making postgrest's enum unreachable through the umbrella library. It also confused the capability matrix, which keys symbols by bare name: it registered `HttpMethod` for functions and `HttpMethod.value` for postgrest as if they were one type. The stringly-typed side had its own cost. `storage_client` compared `method != 'GET'` to decide whether to set a JSON content type, in both its `Fetch` helper and its Iceberg REST catalog. ## What changed One `HttpMethod` in `supabase_common`, taking postgrest's shape: the six methods plus the `value` getter that `functions_client` used to open-code. Public surface: - `functions_client` and `postgrest` re-export the shared enum, so callers of either library see no change. - `supabase.dart` exports postgrest whole again. - `functions.invocation.method_override` registers `FunctionsClient.invoke`, since the shared enum lives in `supabase_common`, which `.sdk-parse-ignore` excludes from the scanned public API surface. A note records that the Dart enum also offers `head`, which supabase-js does not expose for function invocation. - The `edge_functions` example's "every HTTP method" test skips `head`, whose response carries no body for the echo function to reflect, and asserts against `method.value`. Internal adoption, no public API change since both declarations were `@internal`: - `gotrue`: `RequestMethodType` is gone and its roughly 65 call sites use the shared enum. `GotrueFetch`'s dispatch switch gains a `head` branch to stay exhaustive over six values, wired to `http.Client.head`, and now skips the JSON content type for `head` as well as for `get`. No gotrue call site issues a HEAD today, so that branch is currently unexercised. - `storage_client`: `Fetch`'s private request helpers and the Iceberg catalog's `_request` take an `HttpMethod` instead of a `String`, the wire strings come from `.value`, and the content-type branch compares enum values. The public wrapper method names (`get`, `post`, `head`, and so on) are unchanged. Every log line that interpolates a method uses `.value`, so they keep printing `GET` rather than the enum's default `toString`. The `supabase_common` pins stay at `0.1.2`; `melos version` rewrites dependents' pins at release time. ## Merge order This targets `main` directly and does not depend on #1673 or #1677, though it came out of the same review thread. #1673 previously registered `HttpMethod` and `HttpMethod.value`, which this PR makes dangling; those registrations have been pruned on that branch, so the two can merge in either order. ## Verification - `dart analyze` clean across all packages and examples. - `dcm analyze` reports no issues in any file this PR touches. - Suites pass against a local Supabase stack, at the `--concurrency=1` the workflow uses: `gotrue` (479), `storage_client` (210), `postgrest` (196), `functions_client` (48). - The compliance symbol, drift and schema checks pass locally with `main` as the base. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added a shared `HttpMethod` API covering GET, HEAD, POST, PUT, PATCH, and DELETE. - Made `HttpMethod` available through the relevant client libraries. - Added explicit HEAD request support where applicable. - **Bug Fixes** - Improved HTTP method serialization for requests. - Corrected request content handling for GET and HEAD calls. - Updated integration coverage to accurately validate supported methods. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Contributor
Author
The PR description is a little bit misdirecting, the main clean-up reason is that we shouldn't have classes named in a generic fashion like that in the repo, the class names should entail what they are for.
That would be a good improvement, but I don't think we need to do it until it becomes an actual problem.
They are not. |
…internal ones The class was only ever exported under its RealtimeConstants alias, so give it that name directly instead of the ambiguous Constants, which five other packages also declare and which the capability matrix cannot tell apart in its flat symbol registry. defaultHeaders, defaultHeartbeatIntervalMs and wsCloseNormal are only used inside realtime_client, so mark them @internal, matching the other packages whose default header maps are already internal. defaultTimeout and defaultConnectionCloseTimeout stay public since supabase reads them when merging RealtimeClientOptions.
Six packages declared a class called Constants, so a symbol like Constants.defaultHeaders said nothing about which package it belonged to, in the capability matrix or when reading the code. Each is now named after its package, and the file holding it is named to match. The gotrue and realtime_client constants.dart files keep the enums they also declared; only the class moved out into its own file.
spydon
force-pushed
the
chore/realtime-constants-internal
branch
from
August 10, 2026 12:31
e89a942 to
b6d993b
Compare
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
2 participants
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.
Stacked on #1673, addressing #1673 (comment).
Why
Six packages declared a class named
Constants:functions_client,gotrue,realtime_client,storage_client,supabaseandsupabase_flutter. The capability matrix keys symbols by bare name, so an entry likeConstants.defaultHeaderscould not say which package it meant. The same ambiguity applies to anyone reading the code across packages.What changed
Each class is named after its package, and the file declaring it is named to match:
functions_clientFunctionsConstantssrc/functions_constants.dartgotrueGoTrueConstantssrc/gotrue_constants.dartrealtime_clientRealtimeConstantssrc/realtime_constants.dartstorage_clientStorageConstantssrc/storage_constants.dartsupabaseSupabaseConstantssrc/supabase_constants.dartsupabase_flutterSupabaseFlutterConstantssrc/supabase_flutter_constants.dartgotrueandrealtime_clientkeep aconstants.dart, since both also declare enums there. Only the class moved out into its own file, soconstants.dartnow holds exactly the enums and, forgotrue,ApiVersions.gotrue.dartno longer needshide Constantson its export as a result.realtime_clientholds the only public class of the six, so it changed the most:typedef RealtimeConstants = Constantsis gone and the class carries that name directly.realtime_client.dartonly ever exported the alias, so the name consumers write is unchanged.defaultHeaders,defaultHeartbeatIntervalMsandwsCloseNormalare now@internal. Nothing outside the package reads them, and the other five packages' default header maps were already internal. This is the breaking part: code outside the package that reached for them will now getinvalid_use_of_internal_member.defaultTimeoutanddefaultConnectionCloseTimeoutstay public, becausesupabasereads both when filling in unsetRealtimeClientOptions.In
sdk-compliance.yaml, the three now-internal symbols are dropped,realtime.configuration.heartbeat_intervalloses itssupporting_symbolslist along with them, the duplicatedConstantsandConstants.defaultTimeoutpair is removed from the top-level list, andRealtimeConstants.defaultConnectionCloseTimeoutandRealtimeConstants.defaultTimeoutare registered under the new name.Verification
dart analyzeclean across all packages and examples.--concurrency=1the workflow uses:gotrue(479),realtime_client(205, integration included),storage_client(210),supabase(134),supabase_flutter(65),functions_client(48).Summary by CodeRabbit