-
Notifications
You must be signed in to change notification settings - Fork 25
feat: add configurable GraphQL request timeouts #289
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
base: next
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,12 +1,14 @@ | ||||||||||||||||||
| import fs from "node:fs"; | ||||||||||||||||||
| import os from "node:os"; | ||||||||||||||||||
| import path from "node:path"; | ||||||||||||||||||
| import { parseGraphqlTimeoutOption } from "./number-options.js"; | ||||||||||||||||||
| import { getStoredToken } from "./token-storage.js"; | ||||||||||||||||||
|
|
||||||||||||||||||
| export interface CommandOptions { | ||||||||||||||||||
| apiToken?: string; | ||||||||||||||||||
| compact?: boolean; | ||||||||||||||||||
| fields?: string[]; | ||||||||||||||||||
| graphqlTimeoutMs?: number; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| export type TokenSource = "flag" | "env" | "stored" | "legacy"; | ||||||||||||||||||
|
|
@@ -55,3 +57,18 @@ export function getApiToken(options: CommandOptions): string { | |||||||||||||||||
| const { token } = resolveApiToken(options); | ||||||||||||||||||
| return token; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| export function resolveGraphqlTimeoutMs( | ||||||||||||||||||
| options: CommandOptions, | ||||||||||||||||||
| ): number | undefined { | ||||||||||||||||||
| if (options.graphqlTimeoutMs !== undefined) { | ||||||||||||||||||
| return options.graphqlTimeoutMs; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| const environmentValue = process.env["LINEAR_GRAPHQL_TIMEOUT_MS"]; | ||||||||||||||||||
| if (environmentValue) { | ||||||||||||||||||
| return parseGraphqlTimeoutOption(environmentValue); | ||||||||||||||||||
| } | ||||||||||||||||||
|
Comment on lines
+68
to
+71
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Paired change for the parser above, so the env var names itself in the error.
Suggested change
|
||||||||||||||||||
|
|
||||||||||||||||||
| return undefined; | ||||||||||||||||||
| } | ||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,15 +1,27 @@ | ||||||||||||||||||||||||||
| import type { Command } from "commander"; | ||||||||||||||||||||||||||
| import { GraphQLClient } from "../client/graphql-client.js"; | ||||||||||||||||||||||||||
| import { type CommandOptions, getApiToken } from "./auth.js"; | ||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||
| GraphQLClient, | ||||||||||||||||||||||||||
| setGraphqlRequestTimeoutMs, | ||||||||||||||||||||||||||
| } from "../client/graphql-client.js"; | ||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||
| type CommandOptions, | ||||||||||||||||||||||||||
| getApiToken, | ||||||||||||||||||||||||||
| resolveGraphqlTimeoutMs, | ||||||||||||||||||||||||||
| } from "./auth.js"; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| export type { CommandOptions }; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| export interface CommandContext { | ||||||||||||||||||||||||||
| gql: GraphQLClient; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| export function configureGraphqlRequestTimeout(options: CommandOptions): void { | ||||||||||||||||||||||||||
| setGraphqlRequestTimeoutMs(resolveGraphqlTimeoutMs(options)); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| export function createContext(options: CommandOptions): CommandContext { | ||||||||||||||||||||||||||
| const token = getApiToken(options); | ||||||||||||||||||||||||||
|
Comment on lines
+18
to
23
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not blocking. I read your note about keeping this process-global rather than threading it through the client API, and I'm fine with that. It matches Where it gets called is what I'd push on.
Suggested change
with the hook in program.hook("preAction", async (_thisCommand, actionCommand) => {
const rootOpts = getRootOpts(actionCommand);
setOutputOptions(rootOpts);
configureRequestTimeout(rootOpts);
await maybeNotifyUpdate(pkg.version);
});Both One trade-off, since you wrote a test pinning the current behaviour: running the hook first flips the precedence in |
||||||||||||||||||||||||||
| configureGraphqlRequestTimeout(options); | ||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||
| gql: new GraphQLClient(token), | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -29,3 +29,21 @@ export function parseEstimateOption(raw: string): number { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return value; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export function parseGraphqlTimeoutOption(raw: string): number { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const value = parseStrictNonNegativeInteger(raw); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (value === null || value < 1) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw invalidParameterError( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "--graphql-timeout-ms", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "must be a positive integer", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (value > 2_147_483_647) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw invalidParameterError( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "--graphql-timeout-ms", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "must not exceed 2147483647", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return value; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+33
to
+49
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Worth folding into the rename pass. One snag I hit trying it: a defaulted I lifted the timer bound into a constant while I was in there.
Suggested change
On the built CLI that gives: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -33,6 +33,7 @@ import { | |||||||||||||||||||||
| interceptParseErrors, | ||||||||||||||||||||||
| } from "./common/cli-errors.js"; | ||||||||||||||||||||||
| import { getRootOpts } from "./common/context.js"; | ||||||||||||||||||||||
| import { parseGraphqlTimeoutOption } from "./common/number-options.js"; | ||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The import goes with the rename.
Suggested change
|
||||||||||||||||||||||
| import { parseFieldsList, setOutputOptions } from "./common/output.js"; | ||||||||||||||||||||||
| import { maybeNotifyUpdate } from "./common/update-notifier.js"; | ||||||||||||||||||||||
| import { | ||||||||||||||||||||||
|
|
@@ -46,6 +47,11 @@ program | |||||||||||||||||||||
| .description("CLI for Linear.app with JSON output") | ||||||||||||||||||||||
| .version(pkg.version) | ||||||||||||||||||||||
| .option("--api-token <token>", "Linear API token") | ||||||||||||||||||||||
| .option( | ||||||||||||||||||||||
| "--graphql-timeout-ms <ms>", | ||||||||||||||||||||||
| "GraphQL request timeout in milliseconds", | ||||||||||||||||||||||
| parseGraphqlTimeoutOption, | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
|
Comment on lines
+50
to
+54
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GraphQL is how we happen to talk to Linear. It isn't something a person using this CLI should need to know about, and nothing else on the surface mentions it: Can we call it Nothing is released yet. That means this costs a find-and-replace today, and a deprecation cycle if we leave it. It's the only reason I'm not just approving.
Suggested change
Everything else is mechanical: |
||||||||||||||||||||||
| .option("--compact", "emit single-line JSON (no indentation)") | ||||||||||||||||||||||
| .option( | ||||||||||||||||||||||
| "--fields <list>", | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accurate per attempt, but it reads as a per-command promise, and that's where people will get bitten.
isRetryable()matches on"timed out", which is exactly what we throw on abort atgraphql-client.ts:159. So a timeout is itself retryable: four attempts, plus 500ms + 1s + 2s of backoff. Someone sets 5000 wanting a fast fail in a script and waits about 23 seconds.The retry behaviour is fine and predates this PR. It's the sentence that needs to be honest.