diff --git a/rules/brightscript-style.mdc b/rules/brightscript-style.mdc new file mode 100644 index 0000000..50f295b --- /dev/null +++ b/rules/brightscript-style.mdc @@ -0,0 +1,288 @@ +--- +description: BrightScript/BrighterScript coding style conventions +globs: "**/*.brs,**/*.bs" +alwaysApply: false +--- + +# BrightScript / BrighterScript Style Guide + +## Lint + +The project uses `bslint`. Run `npm run lint` to check. Key rules: + +- `inline-if-then`: **never** +- `block-if-style`: **then** +- `no-print`: info +- `no-todo`: info +- `stop`: warn + +## if-then + +Always use multi-line `if`/`then`/`end if`. No one-liners. No wrapping conditions in parentheses unless complex AND/OR logic. Add a blank line before and after the block. + +```brightscript +' GOOD +if a > 2 then + doSomethingCool() +end if + +' BAD +if a > 2 then doSomethingCool() +``` + +## Type Annotations + +Specify argument and return types. Avoid `as dynamic` when possible. Do not annotate subs with `void`. + +```brightscript +' GOOD +function somethingNeat(x as integer) as boolean + return x % 2 +end function + +sub executeSomething() + ' no void annotation needed +end sub + +' BAD +function somethingNeat(x) + return x % 2 +end function +``` + +## Returns + +Combine `return` with the expression when simple. Keep one blank line above `return` when preceded by other statements. + +```brightscript +' GOOD — simple +function isGreater(x as integer) as boolean + return x > 2 +end function + +' GOOD — blank line before return +function getSomething(x as integer) as boolean + someGlobal = m.someGlobal + if someGlobal = invalid then + return false + end if + + return someGlobal.someBoolean +end function + +' BAD — unnecessary temp variable +function isGreater(x as integer) as boolean + y = x > 2 + return y +end function +``` + +## Function / Sub Naming + +Use camelCase. For intrinsic objects, prefix private functions with `_`. For library-style `.brs` files, prefix with `FileName_` to avoid naming collisions. + +```brightscript +' GOOD — intrinsic object with private function +function SomeApi() as object + obj = {} + obj._doesUserExist = function() as boolean + return true + end function + obj.doApiThing = sub() + if m._doesUserExist() then + ' do something + end if + end sub +end function + +' GOOD — library file (SomeApi.brs) +function SomeApi_doesUserExist() as boolean + return true +end function + +' BAD — no prefix, risks naming collision +function doesUserExist() as boolean + return true +end function +``` + +## Guard Clauses + +Check invalid/missing values early and return before the main logic. Keeps code flat and readable. + +```brightscript +' GOOD +sub somethingCool(content as object) + if content = invalid or content.url = invalid then + return + end if + + someOtherFunction(content.url) +end sub + +' BAD — no guards, content may be invalid +sub somethingCool(content as object) + someOtherFunction(content.url) +end sub +``` + +## Arithmetic + +Keep spaces around operators. + +```brightscript +timeout = someTimeoutMs * 1000 +timeoutMs = someTimeoutSeconds / 1000 +``` + +## Built-in Casing + +Use camelCase forms of built-in functions: `getInterface`, `lookup`, `createObject`, etc. + +For Roku component/object methods, use PascalCase as documented in the Roku API: `getFailureReason`, `getMessagePort()`, `setMessagePort()`, etc. + +```brightscript +' GOOD +iface = getInterface(obj, "ifAssociativeArray") +reason = r.getFailureReason + +' BAD +iface = GetInterface(obj, "ifAssociativeArray") +reason = r.getfailurereason +``` + +## BrighterScript SG Node Calls + +When defining functions called via `@.` (callfunc), always include `params = invalid as dynamic` in the signature. The `@.` operator transpiles to `callFunc(..., invalid)` and omitting the param can cause silent failures. + +```brightscript +' GOOD +function doSomething(params = invalid as dynamic) as object + ' ... +end function + +' BAD — @. calls will silently fail +function doSomething() as object + ' ... +end function +``` + +## Dynamic AA Key Access + +Prefer bracket notation `aa[key]` over `aa.lookup(key)` when checking for `invalid` afterward. + +```brightscript +' GOOD +value = config[key] +if value <> invalid then + ' use value +end if + +' BAD +value = config.lookup(key) +if value <> invalid then + ' use value +end if +``` + +## Null Safety + +Optional chaining (`?.`) works in both `.brs` and `.bs` files. Null coalescing (`??`) is BrighterScript-only and must not appear in `.brs` files. + +```brightscript +' GOOD — optional chaining works in .brs and .bs +value = m.config?.timeout + +' GOOD — plain BrightScript fallback (use in .brs instead of ??) +timeout = args.timeout +if timeout = invalid then + timeout = m.config?.defaultTimeout +end if + +' GOOD — ?? is fine in .bs files only +' timeout = args.timeout ?? m.config?.defaultTimeout + +' BAD — ?? in a .brs file (will not compile) +' timeout = args.timeout ?? m.config?.defaultTimeout +``` + +## Progressive AA Construction + +When multiple branches produce similar associative arrays, build a single AA with the common fields first, then layer conditional fields with dot assignment. Assign the AA once at the end. + +```brightscript +' GOOD — build once, layer conditionally, assign once +result = { + "success": isSuccess, + "code": statusCode +} + +if isSuccess then + result.body = responseBody +else + result.error = errorMessage +end if + +m.callback = result + +' BAD — duplicate AA structures across branches +if isSuccess then + m.callback = { + "success": true, "code": statusCode, "body": responseBody + } +else + m.callback = { + "success": false, "code": statusCode, "error": errorMessage + } +end if +``` + +## Resolve-then-Use + +When a value has a default that may be overridden by a condition, assign the default first, override in the branch, then use the resolved value once. Avoids duplicating downstream logic. + +```brightscript +' GOOD — resolve, then use once +timeout = defaultTimeout +if config.timeout <> invalid then + timeout = config.timeout +end if + +request.timeout = timeout + +' BAD — duplicates the assignment with different values +if config.timeout <> invalid then + request.timeout = config.timeout +else + request.timeout = defaultTimeout +end if +``` + +## Preprocessor Conditionals + +`#if` only accepts bare `bs_const` names, `true`, or `false`. No operators (`not`, `=`, `<>`). To negate a const, use an empty `#if` with `#else`: + +```brightscript +' GOOD — negate via #else +#if isUnitTest +#else + print "only in non-test builds" +#end if + +' BAD — not supported +#if not isUnitTest + print "only in non-test builds" +#end if +``` + +## roku-requests Response Properties + +The `roku-requests` response AA stores results of `roUrlEvent` methods as **string properties**, not callable functions. Access them without parentheses: + +```brightscript +' GOOD +failureReason = r.getFailureReason + +' BAD — runtime error: "Function Call Operator attempted on non-function" +failureReason = r.getFailureReason() +``` diff --git a/rules/project-architecture.mdc b/rules/project-architecture.mdc new file mode 100644 index 0000000..3bb759c --- /dev/null +++ b/rules/project-architecture.mdc @@ -0,0 +1,80 @@ +--- +description: roku-requests project architecture and structure +alwaysApply: true +--- + +# Project Architecture + +roku-requests is a BrightScript HTTP library for Roku, modeled after Python's `requests`. It provides a simple API for HTTP requests with query string parameters, custom headers, JSON handling, timeouts, retries, GET response caching, and SSL certificate verification. + +## Directory Structure + +- `src/source/Requests.brs` — The entire library (single file). Installed via ropm or manual copy. +- `test-project/` — Embedded test harness and sample Roku channel + - `manifest` — Roku channel manifest + - `package.json` — Depends on `rr: "file:../"` (local package reference) + - `components/TestsScene.xml` — Rooibos test scene + - `source/main.brs` — Entry point; initializes Rooibos when `args.RunTests = "true"` + - `source/rooibos.cat.brs` — Bundled Rooibos test framework + - `source/testConfig.json` — Rooibos configuration + - `source/tests/` — Test suites (`RequestsTests.brs`, `RequestsUtilsTests.brs`, `RequestsQueryStringTests.brs`, `RequestsHeadersTests.brs`, `RequestsCacheTest.brs`) + - `source/roku_modules/rr/Requests.cat.brs` — ropm-installed copy (functions prefixed `rr_`) +- `bsconfig.json` — BrighterScript config (rootDir: `src`, bslint plugin) +- `.github/workflows/build.yml` — CI: validate + npm release on version tags + +## Build & Validation + +- **Config:** `bsconfig.json` sets `rootDir: "src"` with `@rokucommunity/bslint` plugin. +- **Validate:** `npm run validate` → `bsc --create-package=false --copy-to-staging=false` +- **Preversion hook:** `npm run validate` runs automatically before `npm version`. + +## Test Infrastructure + +- **Framework:** Rooibos (v0.1.0), bundled in `test-project/source/rooibos.cat.brs` +- **Entry:** `main.brs` calls `Rooibos__Init(args, SetupGlobals, AddTestUtils)` when `args.RunTests = "true"` +- **Test scene:** `test-project/components/TestsScene.xml` +- **Suites:** + - `RequestsTests.brs` — Integration tests (URL, status codes, retries, JSON, query params, headers, redirects, POST form/json, cache) + - `RequestsUtilsTests.brs` — `Requests_Utils_inString` utility + - `RequestsQueryStringTests.brs` — Query string building and appending + - `RequestsHeadersTests.brs` — Header handling + - `RequestsCacheTest.brs` — Cache miss, key/MD5, put/get/delete, expiry, bad data + +## Public API + +- `Requests()` — Factory returning `{ request, get, post }` +- `Requests().get(url, args)` — HTTP GET +- `Requests().post(url, args)` — HTTP POST +- `Requests().request(method, url, args)` — Generic method (PUT, DELETE, HEAD, OPTIONS) + +### Request Options (`args`) + +- `params` — Query string AA +- `headers` — Headers AA +- `data` — Raw body string +- `json` — AA serialized as JSON body +- `timeout` — Milliseconds (default 30000) +- `retryCount` — Retry count on failure +- `verify` — SSL cert path (default `common:/certs/ca-bundle.crt`) +- `useCache` — Enable GET response caching (default true) +- `cacheSeconds` — Manual cache TTL +- `parseJson` — Parse response as JSON (default true) +- `parseJsonFlags` — Flags for `ParseJson()` + +### Response Object + +- `ok`, `statusCode`, `url`, `text`, `body`, `headers`, `headersArray` +- `json` — Parsed JSON (when `parseJson` enabled) +- `cacheHit`, `timesTried`, `timedOut` +- `GetSourceIdentity`, `GetFailureReason`, `target_ip` + +## Installation + +**ropm:** `ropm install roku-requests` (functions prefixed `rr_`) + +**Manual:** Copy `src/source/Requests.brs` into your project's `source/` directory. + +## CI/CD + +1. **ci job:** Checkout → Node 16.15.1 → `npm ci` → `npm run validate` +2. **npm-release job:** On version tags (`refs/tags/v*`) → `npm pack` → GitHub release → `npm publish` diff --git a/src/source/Requests.brs b/src/source/Requests.brs index 581c782..7cf1316 100644 --- a/src/source/Requests.brs +++ b/src/source/Requests.brs @@ -54,10 +54,10 @@ function Requests_request(method, url as String, args as Object) if args.json <> invalid and type(args.json) = "roAssociativeArray" _json = FormatJson(args.json) end if - if args.timeout <> invalid and (type(args.timeout) = "Integer" or type(args.timeout) = "roInteger") + if args.timeout <> invalid and (type(args.timeout) = "Integer" or type(args.timeout) = "roInteger" or type(args.timeout) = "roInt") _timeout = args.timeout end if - if args.retryCount <> invalid and (type(args.retryCount) = "Integer" or type(args.retryCount) = "roInteger") + if args.retryCount <> invalid and (type(args.retryCount) = "Integer" or type(args.retryCount) = "roInteger" or type(args.retryCount) = "roInt") _retryCount = args.retryCount end if if args.verify <> invalid and (type(args.verify) = "String" or type(args.verify) = "roString") @@ -66,7 +66,7 @@ function Requests_request(method, url as String, args as Object) if args.useCache <> invalid and (type(args.useCache) = "Boolean" or type(args.useCache) = "roBoolean") _useCache = args.useCache end if - if args.cacheSeconds <> invalid and (type(args.cacheSeconds) = "Integer" or type(args.cacheSeconds) = "roInteger") + if args.cacheSeconds <> invalid and (type(args.cacheSeconds) = "Integer" or type(args.cacheSeconds) = "roInteger" or type(args.cacheSeconds) = "roInt") _cacheSeconds = args.cacheSeconds end if if args.parseJson <> invalid and (type(args.parseJson) = "Boolean" or type(args.parseJson) = "roBoolean") @@ -163,15 +163,17 @@ function Requests_run(method, url, headers, data, timeout, retryCount, verify, p responseEvent = invalid requestDetails = { timesTried : 0, + timedOut : false, parseJson : parseJson, parseJsonFlags: parseJsonFlags } 'while we still have try times while retryCount >= 0 - 'deincrement the number of retries + 'decrement the number of retries retryCount = retryCount - 1 requestDetails.timesTried = requestDetails.timesTried + 1 + requestDetails.timedOut = false sent = invalid @@ -194,6 +196,7 @@ function Requests_run(method, url, headers, data, timeout, retryCount, verify, p event = invalid + port = urlTransfer.getPort() while true and cancel_and_return = false if m.top <> invalid @@ -202,13 +205,19 @@ function Requests_run(method, url, headers, data, timeout, retryCount, verify, p end if end if - event = urlTransfer.GetPort().GetMessage() - - if type(event) = "roUrlEvent" + remainingMs = timeout_call - clock.totalMilliseconds() + if remainingMs <= 0 exit while end if - if clock.TotalMilliseconds() > timeout_call + waitMs = remainingMs + if waitMs > 500 then + waitMs = 500 + end if + + event = wait(waitMs, port) + + if type(event) = "roUrlEvent" exit while end if end while @@ -226,16 +235,20 @@ function Requests_run(method, url, headers, data, timeout, retryCount, verify, p ? "[http] Will Retry ", retryCount end if else - if m.cancel_and_return = true + if cancel_and_return = true ? "[http] Killing the Task" exit while else - 'We timed out so we should cancel the request ? "[http] Event Timed Out" + requestDetails.timedOut = true urlTransfer.AsyncCancel() - 'Exponential backoff timeouts timeout = timeout * 2 ? "[http] Timeout=", timeout + ' Create a fresh roUrlTransfer so the next attempt + ' doesn't race with the in-flight cancel. + urlTransfer = RequestsUrlTransfer(true, true, verify) + urlTransfer.setUrl(url) + urlTransfer.setHeaders(headers) end if end if end if @@ -443,6 +456,7 @@ function Requests_response(urlTransfer as Object, responseEvent as Object, reque rr = {} rr.timesTried = requestDetails.timesTried + rr.timedOut = requestDetails.timedOut rr.url = urlTransfer.GetUrl() rr.ok = false rr.cacheHit = false @@ -478,8 +492,8 @@ function RequestsUrlTransfer(enableEncodings as Boolean, retainBodyOnError as Bo _urlTransfer = CreateObject("roUrlTransfer") _urlTransfer.SetPort(CreateObject("roMessagePort")) - _urlTransfer.EnableEncodings(enableEncodings) - _urlTransfer.RetainBodyOnError(retainBodyOnError) + _urlTransfer.EnableEncodings(enableEncodings) + _urlTransfer.RetainBodyOnError(retainBodyOnError) if verify <> "" _urlTransfer.SetCertificatesFile(verify) _urlTransfer.InitClientCertificates()