-
Notifications
You must be signed in to change notification settings - Fork 7
SDK: auth.broadcast is not deprecated, and a guard so it is not misread again #1378
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
101 changes: 101 additions & 0 deletions
101
packages/sdk/src/modules/core/custom-broadcast-usage.spec.ts
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| import { readdirSync, readFileSync, statSync } from "node:fs"; | ||
| import { join } from "node:path"; | ||
| import ts from "typescript"; | ||
| import { describe, expect, it } from "vitest"; | ||
|
|
||
| /** | ||
| * `auth.broadcast` is the caller-supplied broadcaster, not "the keychain path". | ||
| * | ||
| * Four mutations read it as though it were the latter: they checked | ||
| * `auth?.broadcast` and threw when it was missing. Every field on `AuthContext` | ||
| * is optional, `broadcast?` included, so an `AuthContextV2` satisfies the type | ||
| * structurally and the check compiled fine everywhere. Each site then failed | ||
| * only when a real user reached it, which is how four accumulated before one | ||
| * surfaced as a Sentry issue. | ||
| * | ||
| * The type system cannot express this, so a scan is what is left. It reads the | ||
| * syntax tree rather than the text, because `auth.broadcast` appears in prose | ||
| * throughout these files and a text search reports the comments explaining why | ||
| * not to use it. | ||
| */ | ||
|
|
||
| const MODULES = join(__dirname, ".."); | ||
|
|
||
| /** | ||
| * Where reading `auth.broadcast` is the point rather than a mistake. | ||
| * | ||
| * `use-broadcast-mutation` owns the `custom` branch of the fallback chain, which | ||
| * is the whole feature. Anything else added here is an admission, so it needs a | ||
| * reason next to it. | ||
| */ | ||
| const SANCTIONED: Record<string, string> = { | ||
| "core/mutations/use-broadcast-mutation.ts": | ||
| "owns case 'custom', the last link of the default fallback chain", | ||
| "core/mutations/broadcast-json.ts": | ||
| "first branch of its own fallback chain, kept so V1 callers still work", | ||
| }; | ||
|
|
||
| function sourceFiles(dir: string): string[] { | ||
| return readdirSync(dir).flatMap((entry) => { | ||
| const path = join(dir, entry); | ||
| if (statSync(path).isDirectory()) return sourceFiles(path); | ||
| return /\.ts$/.test(path) && !/\.spec\.ts$/.test(path) ? [path] : []; | ||
| }); | ||
| } | ||
|
|
||
| /** True when the file reads `.broadcast` off something named like an auth ctx. */ | ||
| function readsAuthBroadcast(file: string): boolean { | ||
| const sf = ts.createSourceFile( | ||
| file, | ||
| readFileSync(file, "utf8"), | ||
| ts.ScriptTarget.Latest, | ||
| true, | ||
| ts.ScriptKind.TS | ||
| ); | ||
|
|
||
| let found = false; | ||
| const visit = (node: ts.Node): void => { | ||
| if ( | ||
| ts.isPropertyAccessExpression(node) && | ||
| node.name.text === "broadcast" && | ||
| /^auth$/i.test(node.expression.getText(sf).replace(/[?!]/g, "")) | ||
| ) { | ||
| found = true; | ||
| } | ||
| ts.forEachChild(node, visit); | ||
| }; | ||
| visit(sf); | ||
| return found; | ||
| } | ||
|
|
||
| describe("auth.broadcast is only read where it is the feature", () => { | ||
| const files = sourceFiles(MODULES); | ||
|
|
||
| it("finds the modules to scan", () => { | ||
| // Guards the reader: an empty list would make the sweep vacuous. | ||
| expect(files.length).toBeGreaterThan(50); | ||
| }); | ||
|
|
||
| it("is read only in sanctioned places", () => { | ||
| const offenders = files | ||
| .filter(readsAuthBroadcast) | ||
| .map((f) => f.slice(MODULES.length + 1).split("\\").join("/")) | ||
| .filter((rel) => !(rel in SANCTIONED)); | ||
|
|
||
| expect(offenders).toEqual([]); | ||
| }); | ||
|
|
||
| /** | ||
| * The detector has to detect. A scan that silently matches nothing passes | ||
| * exactly as well as one that works, which is the failure mode this whole | ||
| * class of test invites. | ||
| */ | ||
| it("would catch a new reader, and ignores prose", () => { | ||
| const sanctioned = join(MODULES, "core/mutations/use-broadcast-mutation.ts"); | ||
| expect(readsAuthBroadcast(sanctioned)).toBe(true); | ||
|
|
||
| // A file that only MENTIONS it in a comment must not register. | ||
| const proseOnly = join(MODULES, "core/types/auth.ts"); | ||
| expect(readsAuthBroadcast(proseOnly)).toBe(false); | ||
| }); | ||
| }); | ||
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
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.