Conversation
|
Forgive me. I'm not at first glance seeing why this constitutes a v2 API, which might/probably has breaking changes. There's no changes to call/interface that I can see. What constitutes this being a v2 and what impact that does that have future/current to modules on a v1 API for those versions with regards to core releases? |
|
Currently none of this needs to be v2, but I think is a bit risky to do before v2 so am holding these changes until we are doing a v2. I am anticipating that a v2 may happen as part of the graphics overhaul or 'extending expressions to action options', so want this to be ready for whenever they are. I am expecting that we will have no difficulty supporting both v1 and v2 in companion, itll just need some separate implementation of some chunk of code to support both. |
Currently my hope is that neither of these will make a breaking change necessary. For the expressions I see mainly a challenge with the upgade script, but that will greatly depend on the implementation route we take. |
|
I am expecting that the expressions will 'break' any existing upgrade scripts, but I am happy to be proven wrong. I am wondering if we should call it 2.0 even if there aren't any breaking changes, simply as a indicator to module devs that there is a significant amount of new stuff that they should incorporate. And we might want to encourage (but not require) updating to whatever new preset structure we define. We did briefly discuss replacing the current 'advanced' feedback type with something new, that would definitely be a breaking change, and maybe will land as part of the overhaul (I still need to begin thinking about how those will integrate to the new system, I suspect it will be some bad UX) But again, none of this is set in stone, so this is simply some things I want to be in 2.0 whenever that happens. |
|
This all makes sense to me. Obviously what I'm about to say changes timelines, but I feel like a v2 API would be good to rollout at v4 Companion ... breaking or no. Point being, I think it will be confusing to roll that stuff in at a v4.1 and add v2 API then. I might be overthinking that, but its hard to tell how people are going to react to versioning in the v4 landscape with the store and live updates. |
Julusian
force-pushed
the
feat/2.0
branch
2 times, most recently
from
88bc40f to
54a76e0
Compare
Julusian
force-pushed
the
feat/2.0
branch
2 times, most recently
from
33f488d to
7dcf65d
Compare
Julusian
force-pushed
the
feat/2.0
branch
2 times, most recently
from
5533063 to
ae19264
Compare
…r they are an expression This means that all existing upgrade scripts will need to be updated, or we need to devise some extra handling to avoid this breakage before release
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMonorepo conversion: root renamed and workspaces added for two packages (companion-module-base, companion-module-host). CI/release workflows made package-aware. Large refactor: new typed public APIs, host-side managers, utilities and tests added; legacy runtime, IPC wrapper, many old module-api/host-api files, debounce-fn and related scripts/tests removed. Changes
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
# Conflicts: # yarn.lock
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/companion-module-base/src/module-api/upgrade.ts (1)
168-174:⚠️ Potential issue | 🟠 MajorOptions need to be unwrapped before assigning to style properties.
Great work on the type updates! I did spot something that could cause runtime issues though. Now that
feedback.options[option_key]is typed asExpressionOrValue<JsonValue | undefined> | undefined, assigning it directly tofeedback.style[style_key]means the style property will receive an object like{ value: 0xFF0000, isExpression: false }instead of the raw value (e.g.,0xFF0000).Looking at
CompanionButtonStyleProps, properties likecolor,bgcolor, andpng64all expect plain values (number or string), not wrapped objects. Theas anycast hides this mismatch, but at runtime the style object would have the wrong structure.You'll want to unwrap the value before assigning it to the style, something like:
Suggested fix
for (const [option_key, style_key] of Object.entries(upgrade_rules)) { - if (feedback.options[option_key] !== undefined) { - feedback.style[style_key] = feedback.options[option_key] as any + const optionVal = feedback.options[option_key] + if (optionVal !== undefined) { + feedback.style[style_key] = optionVal.value as any delete feedback.options[option_key] changedFeedbacks.push(feedback).github/workflows/publish.yml (1)
172-181:⚠️ Potential issue | 🔴 Critical🐛 Broken step reference — docs will never be prepared or uploaded
Lines 173 and 178 reference
steps.do-publish.outputs.tag, but there's no step with iddo-publishin thepreparejob. This condition will always evaluate to empty/falsy, so the "Prepare docs" and "Upload docs artifact" steps will be permanently skipped — even for tagged releases where you'd want docs published.It looks like this should reference
needs.determine-package.outputs.taginstead (consistent with how thepublish-docsjob does it on line 235).🔧 Proposed fix
- name: Prepare docs - if: ${{ steps.do-publish.outputs.tag == 'latest' }} + if: ${{ needs.determine-package.outputs.tag == 'latest' }} run: | yarn docs:html - name: Upload docs artifact - if: ${{ steps.do-publish.outputs.tag == 'latest' }} + if: ${{ needs.determine-package.outputs.tag == 'latest' }} uses: actions/upload-pages-artifact@v4 with: path: docs/
🧹 Nitpick comments (29)
packages/companion-module-base/tools/schema-compile.mjs (1)
12-17: The comment on line 14 describes CJS output, but the config generates ESM.Since
esm: trueis set on line 15, the generated code will useexport defaultsyntax rather thanmodule.exports. The comment on line 14 is a bit misleading — looks like it might have been copied from an older version or the AJV docs. Would be nice to update it so future readers aren't confused. 😊📝 Suggested comment update
- // The generated code will have a default export: - // `module.exports = <validateFunctionCode>;module.exports.default = <validateFunctionCode>;` + // The generated code will have a default export: + // `export default <validateFunctionCode>;`packages/companion-module-base/src/module-api/upgrade.ts (1)
253-260: Tiny nit: untrimmed value used for simple variable expressions.On line 259,
oldValRaw(the untrimmed original) is used as the expression value even when the simple-variable detection was done ontrimmedStr. If the user had leading/trailing whitespace like$(foo), that whitespace would carry over into the expression. Probably harmless, but usingtrimmedStrhere might be slightly tidier.packages/companion-module-base/src/helpers/__tests__/tcp.spec.ts (1)
85-85: Nice catch on the typo fix! 👍The correction from "propogation" to "propagation" looks good. Thanks for tidying this up!
As a small note — there's quite a lot of commented-out test code in this file. If those tests are no longer relevant to the reworked TCP helper API, it might be nice to clean them out at some point to reduce noise. No rush though!
packages/companion-module-base/src/common/json-value.ts (1)
7-7: Tiny nit: the removed space before{slightly hurts readability.The doc comment example now reads
extends JsonObject{ … }— conventionally TypeScript style would keep a space before the brace:extends JsonObject { … }. This is super minor, but if you're touching this line anyway, might be nice to keep the space. Totally up to you! 😊Suggested tweak
-This type can be useful to enforce some input to be JSON-compatible or as a super-type to be extended from. Don't use this as a direct return type as the user would have to double-cast it: `jsonObject as unknown as CustomResponse`. Instead, you could extend your CustomResponse type from it to ensure your type only uses JSON-compatible types: `interface CustomResponse extends JsonObject{ … }`. +This type can be useful to enforce some input to be JSON-compatible or as a super-type to be extended from. Don't use this as a direct return type as the user would have to double-cast it: `jsonObject as unknown as CustomResponse`. Instead, you could extend your CustomResponse type from it to ensure your type only uses JSON-compatible types: `interface CustomResponse extends JsonObject { … }`.tsconfig.base.json (1)
4-5: Minor: inconsistent indentation and leftover commented-out optionHey there! 👋 Looks like line 4 uses spaces for indentation while the rest of the file uses tabs. Also, the commented-out
"importHelpers": falseon line 5 is superseded by the active"importHelpers": trueon line 11 — might be nice to clean that up to avoid confusion for future readers.Suggested cleanup
{ "extends": "@tsconfig/node22/tsconfig.json", "compilerOptions": { - "composite": true, - // "importHelpers": false, + "composite": true, "types": ["node"],packages/companion-module-host/tsconfig.json (1)
5-13: Consider aligningmodule/moduleResolutionwith the sibling packageJust a heads up —
packages/companion-module-base/tsconfig.jsonexplicitly sets"module": "node20"and"moduleResolution": "node16", but this host package config doesn't, so it'll inherit whatever@tsconfig/node22defaults to (viatsconfig.base.json). That's probably fine, but you might want to make it explicit here too for consistency and to avoid surprises if the base preset changes.Totally up to you though — no strong feelings here! 😊
tsconfig.tests.json (1)
10-11: Minor: indentation inconsistency oncompositeSame as in
tsconfig.base.json— line 11 uses spaces instead of tabs for"composite": true. Just a small nit for consistency! 🙂Suggested fix
"compilerOptions": { - "composite": true, + "composite": true,packages/companion-module-base/tsconfig.json (2)
1-2: Inconsistent base config: extends@tsconfig/node22directly vstsconfig.base.jsonJust noticed this package extends
@tsconfig/node22/tsconfig.jsondirectly, whilepackages/companion-module-host/tsconfig.jsonextends../../tsconfig.base.json(which in turn extends@tsconfig/node22). Both end up with similar settings since you duplicate the options here, but using a shared base would reduce drift between packages over time.Totally a "nice to have" — the current setup works fine! 👍
5-6: Minor: same indentation nit oncompositeSpaces instead of tabs on line 6, same as the other tsconfig files. 🙂
.github/workflows/node.yaml (2)
38-41: Single-entry matrix — consider simplifying or expanding.Just a thought: since the matrix only contains
[22.x], you could either drop the matrix strategy entirely, or consider adding another version (like24.xwhen it's available) to get broader coverage. Totally fine to leave as-is if you plan to expand later!
65-69: Redundantyarn buildin docs generation step.The "Build Packages" step (line 55-59) already runs
yarn build, so theyarn buildhere on line 68 is doing the same work again. You could safely remove it to save CI time. Also, theif: matrix.node-version == '22.x'condition on line 66 is always true now since the matrix only has22.x— feel free to drop it or keep it as a guard for when the matrix expands.💡 Suggested simplification
- name: Check docs generation - if: matrix.node-version == '22.x' run: | - yarn build yarn docs:html env: CI: truepackages/companion-module-base/README.md (1)
33-33: Consider using descriptive link text instead of "here".Per accessibility best practices (and markdownlint MD059), link text like "here" doesn't convey meaning out of context. Something like "view the detailed generated documentation" as the link text would be friendlier for screen readers and scanners.
✏️ Suggested fix
-You can view detailed generated documentation [here](https://bitfocus.github.io/companion-module-base/). +You can view the [detailed generated documentation](https://bitfocus.github.io/companion-module-base/).packages/companion-module-base/package.json (1)
29-31: Consider clarifying why^22.21is required instead of^22.12Node.js v22.21.0 and v22.22.0 do exist, so the constraint is technically valid. However, since your PR mentions
require(esm)support is available since Node 22.12, setting the floor to 22.21 might be more restrictive than needed. Was this intentional, or would^22.12be sufficient to support the package's requirements? Just want to make sure we're not unnecessarily blocking users with earlier Node 22.x versions! 😊packages/companion-module-base/src/logging.ts (1)
19-22: Consider using level-appropriate console methods in the default sinkCurrently the default sink routes all log levels through
console.log. For better developer experience (especially during unit testing), you might wantwarn→console.warnanderror→console.errorso they show up in stderr and are visually distinct in most terminals/test runners.Totally fine if this is intentional since it's meant to be replaced by the host — just wanted to flag it in case it was an oversight! 😊
💡 Optional: use level-specific console methods
const defaultLoggingSink: LoggingSink = (source, level, message) => { // Default logging implementation for unit testing purposes, this should be replaced by the host application - console.log(`[${level.toUpperCase()}]${source ? ` [${source}]` : ''} ${message}`) + const formatted = `[${level.toUpperCase()}]${source ? ` [${source}]` : ''} ${message}` + switch (level) { + case 'error': + console.error(formatted) + break + case 'warn': + console.warn(formatted) + break + case 'debug': + console.debug(formatted) + break + default: + console.log(formatted) + break + } }packages/companion-module-host/src/internal/actions.ts (1)
241-262: Minor nit: leftoverfbvariable name from feedback codeLines 245 and 268 use
fbas the filter parameter name (e.g.,actions.filter((fb) => ...)), which looks like a copy-paste artifact from feedback handling code. Renaming toactorawould improve readability and consistency with the rest of this file.No big deal at all — just a small readability nit! 😄
✏️ Suggested rename
- if (actionIdSet.size) actions = actions.filter((fb) => actionIdSet.has(fb.actionId)) + if (actionIdSet.size) actions = actions.filter((act) => actionIdSet.has(act.actionId))(Same change in both
subscribeActionsandunsubscribeActions)Also applies to: 264-285
packages/companion-module-base/src/module-api/base.ts (1)
285-304: TheassertNeverfallthrough is fine, but Line 301 is unreachable.Since
assertNeverreturnsnever(and throws), thethis.#logger.info(message)andbreakon lines 301–302 will never execute. This is a common defensive pattern and totally fine — just noting it in case it was unintentional. No action needed unless you'd like to tidy it up.packages/companion-module-base/src/module-api/preset/structure.ts (1)
38-48: Consider whethertemplateValuesshould enforce at least one entry.Right now
templateValuesis typed as an array with no minimum length. A template group with zero values would presumably generate zero presets, which might be confusing. If that's a valid use case, no worries — but if not, you might consider a non-empty tuple type like[...entry, ...entry[]]to catch mistakes at compile time. Just a thought!packages/companion-module-base/src/module-api/shared-udp-socket.ts (2)
123-123: Consider usingthis.once(...)instead ofthis.on(...)for one-shot callbacks.On Line 123 (
bind) and Line 165 (close), the callbacks are added withthis.on(...). Since these events should fire exactly once per socket lifecycle (you can only bind once, close once), usingthis.once(...)would be more semantically accurate and automatically clean up the listener after it fires. Not a bug (the state machine prevents re-entry), but a nice hygiene improvement.✨ Suggested change
- if (callback) this.on('listening', callback) + if (callback) this.once('listening', callback)- if (callback) this.on('close', callback) + if (callback) this.once('close', callback)Also applies to: 165-165
125-142: The.catch(() => null)guards are a pragmatic resilience choice.I can see these are intentionally preventing unhandled promise rejections from crashing modules when user event handlers throw. The pattern is applied consistently across
bind,close, andsend. Just a gentle suggestion: if debugging ever becomes painful, acatch((e) => { /* log if debug */ })with an optional debug-level log could help. Not a priority, just a thought for the future.Also applies to: 167-182, 241-258
packages/companion-module-host/src/internal/presets.ts (1)
84-86: Consider logging the actual error for debugging purposes 🔍Right now, when validation throws, only the preset name is recorded. It might be helpful during development to also log the error message — it could save someone a headache when tracking down a tricky preset issue!
💡 Suggested improvement
} catch (_e) { - presetsFailedValidation.push(presetName) + presetsFailedValidation.push(presetName) + logger.debug(`Preset validation error for "${presetName}": ${_e instanceof Error ? _e.message : _e}`) }packages/companion-module-host/src/internal/__tests__/upgrade.spec.ts (1)
115-115: Minor inconsistency in generic type parameters across assertions 🤓Some assertions use
CompanionStaticUpgradeResult<JsonObject, JsonObject>(e.g., lines 115, 234, 295, 304) while others use<any, any>(e.g., lines 142, 159, 188, 224). Not a correctness issue at all — just thought I'd mention it in case you'd like to align them for consistency. Totally fine to leave as-is!Also applies to: 142-142, 159-159, 188-188, 224-224, 234-234, 295-295, 304-304
packages/companion-module-host/src/instance.ts (2)
274-275: Tiny nit:==vs===inconsistency 🔎Line 274 uses strict equality (
===) while line 275 uses loose equality (==). Both work fine here since you're comparing against a string literal, but it'd be nice to keep them consistent!- hasRecordActionsHandler: typeof this.#instance.handleStartStopRecordActions == 'function', + hasRecordActionsHandler: typeof this.#instance.handleStartStopRecordActions === 'function',
268-271: Consider using the module logger instead ofconsole.trace📝The rest of the codebase uses
createModuleLoggerfor logging (there's even a commented-out logger on line 33!). Usingconsole.tracehere means this error won't go through the same logging pipeline. Might be worth switching to the structured logger if you end up uncommenting it.packages/companion-module-host/src/internal/__tests__/feedback.spec.ts (1)
2-2: Cross-package relative import might be fragile 🧩This import reaches across package boundaries using a deep relative path (
../../../../companion-module-base/src/__mocks__/util.js). In a monorepo, this could break if the directory structure changes. You might consider re-exporting test utilities from the package's public surface or using a shared test-utils package. Just a thought for future maintainability — no rush!packages/companion-module-host/src/internal/feedback.ts (1)
221-223:console.errorvsthis.#logger.errorinconsistency 📋The rest of the class uses
this.#logger.errorfor error logging (e.g., lines 84-86, line 360), but this catch block usesconsole.error. Might be nice to keep it consistent so all logs flow through the same pipeline!- console.error(`Feedback check failed: ${JSON.stringify(feedback)} - ${e?.message ?? e} ${e?.stack}`) + this.#logger.error(`Feedback check failed: ${JSON.stringify(feedback)} - ${e?.message ?? e} ${e?.stack}`)packages/companion-module-base/src/module-api/preset/definition.ts (2)
7-7: Consider usingimport typeforCompanionActionSchemaHey there! 👋 Since this file is purely type definitions with no runtime logic, it might be nice to use
import typehere instead of a value import. This ensures the import is erased at compile time and avoids pulling in the module at runtime — especially relevant now that the library is ESM.Suggested change
-import { CompanionActionSchema } from '../action.js' +import type { CompanionActionSchema } from '../action.js'
81-103: Mixing named properties with a numeric index signature could be surprisingThis is a creative approach to model long-press duration groups alongside the fixed
down/up/rotate_left/rotate_rightproperties! Just a thought — the numeric index signature on line 102 means callers can index with any number, which might make it tricky to iterate or serialize correctly (e.g.,Object.entrieswill return the named properties mixed in with duration-keyed ones). It could also be confusing for contributors who aren't familiar with this pattern.Would it be worth considering a separate property (e.g.,
longPressGroups?: Record<number, ...>) to keep the two concerns distinct? Totally fine to leave as-is if this pattern is well-understood by the team — just flagging it for visibility.packages/companion-module-base/src/host-api/context.ts (1)
61-67:InstanceSharedUdpSocketContextexposes the concreteSharedUdpSocketImplclassThe
sharedUdpSocketHandlersfield referencesSharedUdpSocketImpl(the concrete class) rather than theSharedUdpSocketinterface. If this is deliberate because the host-side genuinely needs access to implementation internals, that's fine! But if the map consumers only need the publicSharedUdpSocketsurface, referencing the interface would keep the coupling looser.packages/companion-module-host/src/context.ts (1)
151-157:updatedConfigandupdatedSecretstyped asunknownlose generic safetyIn
UpgradeActionAndFeedbackInstancesResponse,updatedConfigandupdatedSecretsareunknownrather than parameterized generics. Since this interface isn't generic itself, the config/secrets type information is lost. If this is intentional (e.g., upgrade scripts may produce arbitrary shapes that are only validated later), that's fine! Just wanted to flag it in case parameterizing this interface was on the radar.Possible approach if generics are desired
-export interface UpgradeActionAndFeedbackInstancesResponse { - updatedConfig: unknown - updatedSecrets: unknown +export interface UpgradeActionAndFeedbackInstancesResponse<TConfig = unknown, TSecrets = unknown> { + updatedConfig: TConfig + updatedSecrets: TSecrets updatedActions: UpgradeActionInstance[] updatedFeedbacks: UpgradeFeedbackInstance[] latestUpgradeIndex: number }
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/publish.yml (1)
172-179:⚠️ Potential issue | 🟠 MajorDocs build is guarded by a non-existent step output.
steps.do-publish.outputs.tagisn’t defined in thepreparejob, so these steps never run. Use thedetermine-packageoutputs instead.🔧 Suggested fix
- - name: Prepare docs - if: ${{ steps.do-publish.outputs.tag == 'latest' }} + - name: Prepare docs + if: ${{ needs.determine-package.outputs.tag == 'latest' }} run: | yarn docs:html - - name: Upload docs artifact - if: ${{ steps.do-publish.outputs.tag == 'latest' }} + - name: Upload docs artifact + if: ${{ needs.determine-package.outputs.tag == 'latest' }} uses: actions/upload-pages-artifact@v4 with: path: docs/
🧹 Nitpick comments (13)
packages/companion-module-base/src/__mocks__/util.ts (1)
28-48: Nice addition ofcreateManualPromiseutility!This is a handy helper for tests that need fine-grained control over promise resolution timing. One tiny thought for future consideration: the
isResolvedproperty gets set totrueon both resolution and rejection. If you ever want to distinguish between the two states, you might rename it to something likeisSettled– but honestly, for most test scenarios the current behavior is totally fine and this is a very minor nitpick. Feel free to ignore!packages/companion-module-base/tools/schema-compile.mjs (1)
12-24: Validator generation and bundling looks solid!The approach of generating standalone Ajv code and then bundling it with esbuild is a nice way to avoid runtime Ajv dependencies for consumers. The
--allow-overwriteflag makes sense since you're bundling in-place.A couple of minor suggestions you could consider:
Sync vs async file write: You're mixing async (
$from zx) with sync (fs.writeFileSync). Usingfs.promises.writeFilewould keep everything consistently async, though it's not a big deal for a build script.Error handling: If the bundle step fails, it might be helpful to have clearer messaging. Again, totally optional for a build tool.
Otherwise, this is a clean implementation! Thanks for putting this together. 🎉
💡 Optional: async file write for consistency
-import fs from 'fs' +import fs from 'fs/promises' // ... - fs.writeFileSync(outputPath, moduleCode) + await fs.writeFile(outputPath, moduleCode)packages/companion-module-host/package.json (1)
17-19: Consider broadening the Node version constraintThe
^22.21constraint is quite specific and would exclude Node 22.0-22.20. If there isn't a specific reason for 22.21 specifically, you might consider^22for broader compatibility. (Note:require(esm)has been available since Node 22.0.0, so if that's the concern, any Node 22 version would work.)Just wanted to check in case it was an accidental over-constraint—totally fine if there's a specific reason! 😊
packages/companion-module-base/src/logging.ts (1)
24-27: Consider wrapping sink invocation in try-catch for resilienceIf a custom
SURFACE_LOGGERthrows, it could disrupt the caller's flow. You might want to catch errors here and fall back to the default sink or silently swallow the error to prevent logging from crashing the module. Just a thought for extra robustness!💡 Optional defensive wrapper
function logToSink(source: string | undefined, level: LogLevel, message: string) { const sink = typeof global.SURFACE_LOGGER === 'function' ? global.SURFACE_LOGGER : defaultLoggingSink - sink(source, level, message) + try { + sink(source, level, message) + } catch { + // Fallback if custom sink fails + defaultLoggingSink(source, level, message) + } }.github/workflows/node.yaml (1)
65-69: Minor: Redundant build and conditionA couple of small observations:
- The
yarn buildon line 68 duplicates the build that already ran on line 57 - you could potentially remove it from the docs step.- The condition
matrix.node-version == '22.x'on line 66 is now always true since the matrix only contains22.x. It's harmless but could be simplified.Neither of these is a problem - just tidying opportunities if you're inclined!
💡 Optional cleanup
- name: Check docs generation - if: matrix.node-version == '22.x' run: | - yarn build yarn docs:html env: CI: truepackages/companion-module-host/src/internal/upgrade.ts (1)
104-147: Solid upgrade script execution flowThe logic for passing cloned config/options to upgrade scripts and applying results back is well-structured. The use of
structuredClonefor deep cloning andliteral<Complete<...>>for type-safe construction is a nice pattern.One small observation: the
// TODO - style?comment on line 139 might be worth tracking if style handling for feedbacks needs to be addressed before release.Would you like me to open an issue to track the TODO about style handling in migration feedbacks?
packages/companion-module-host/src/internal/presets.ts (2)
117-133: Consider handling all definition types explicitlyThe logic here handles string references,
type === 'simple'groups, andtype === 'template'groups. This looks complete based on theCompanionPresetGroupunion type, but you might want to add anelsebranch with a debug log for unexpected types—just in case the type system evolves and a new group type is added later.That said, this is totally optional and the current implementation covers all existing cases well! 👍
84-86: Empty catch block hides potential issuesThe
try/catchis good for resilience, but silently catching all exceptions might hide bugs in the validation logic itself. Consider logging a debug message with the actual error for troubleshooting:💡 Suggested enhancement
} catch (_e) { + // Log the actual error for debugging + logger.debug(`Preset validation error for ${presetName}: ${_e}`) presetsFailedValidation.push(presetName) }packages/companion-module-host/src/internal/feedback.ts (1)
221-223: Inconsistent logging methodLine 222 uses
console.errorwhile the rest of the class usesthis.#logger.error. For consistency and to leverage the module logging infrastructure, consider updating this:💡 Suggested fix
.catch((e) => { - console.error(`Feedback check failed: ${JSON.stringify(feedback)} - ${e?.message ?? e} ${e?.stack}`) + this.#logger.error(`Feedback check failed: ${JSON.stringify(feedback)} - ${e?.message ?? e} ${e?.stack}`) })packages/companion-module-base/src/host-api/context.ts (1)
69-73: Consider expanding the TODO for SharedUdpSocketMessageJoinThe TODO on line 72 mentions "more props?" — if you know what properties might be needed (like
addressfor multicast support,reuseAddr, etc.), it might be helpful to capture those thoughts in the comment for future reference.packages/companion-module-base/src/module-api/preset/structure.ts (1)
102-106: Consider adding a JSDoc note about the union type flexibilityThe
definitionsfield accepts eitherCompanionPresetGroup<TManifest>[]orCompanionPresetReference[]. This flexibility is great, but a quick note in the JSDoc explaining when you'd use one vs. the other might help module authors choose the right approach.packages/companion-module-base/src/module-api/action.ts (1)
105-112: Minor typo: Inconsistent generic parameter casingHey there! Nice work on the generic typing improvements — this makes the API much more type-safe! 🎉
I noticed a small inconsistency: the generic parameter is named
Tschemason line 106, but it's referenced asTschemasagain. However, comparing with the pattern used elsewhere (likeTOptions,TConfig,TManifest), it seems likeTSchemas(with capital S) would be more consistent with TypeScript naming conventions.✨ Suggested fix for consistent casing
export type CompanionActionDefinitions< - Tschemas extends Record<string, CompanionActionSchema<CompanionOptionValues>> = Record< + TSchemas extends Record<string, CompanionActionSchema<CompanionOptionValues>> = Record< string, CompanionActionSchema<CompanionOptionValues> >, > = { - [K in keyof Tschemas]: CompanionActionDefinition<Tschemas[K]['options']> | false | undefined + [K in keyof TSchemas]: CompanionActionDefinition<TSchemas[K]['options']> | false | undefined }packages/companion-module-base/src/module-api/base.ts (1)
299-302: Unreachable code afterassertNeverThe
assertNever(level)function is designed to throw an error to ensure exhaustive handling of all cases. Thethis.#logger.info(message)on line 301 will never execute sinceassertNeverthrows. This makes the code a bit confusing for readers.If you want to keep a fallback for future-proofing (in case
assertNeverbehavior changes), you could remove thebreakand let it fall through. Otherwise, removing the unreachable line would clean things up!🧹 Option 1: Remove unreachable code
default: assertNever(level) - this.#logger.info(message) - break }🧹 Option 2: If you want a defensive fallback
default: - assertNever(level) - this.#logger.info(message) - break + // This should never happen if all LogLevel values are handled + this.#logger.info(message)
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/companion-module-base/assets/manifest.schema.json (1)
82-86:⚠️ Potential issue | 🟡 MinorConsider whether
node18should be removed from the manifest schema.The PR notes indicate Node 18 support is being dropped with a shift to ESM and Node 22+ as the target. Currently,
node18appears only in the manifest schema (lines 84-85), so removing it would have no impact elsewhere in the codebase. That said, if backward compatibility with existing module manifests is important during the transition, keeping it could be intentional. Just wanted to make sure it wasn't overlooked! 😊packages/companion-module-base/src/module-api/upgrade.ts (1)
165-176:⚠️ Potential issue | 🟠 MajorType mismatch:
ExpressionOrValue<...>assigned to style property viaas any.
rawValis nowExpressionOrValue<JsonValue | undefined> | undefined, butfeedback.styleexpectsPartial<CompanionFeedbackButtonStyleResult>which has concrete value types (strings, numbers). Theas anycast hides this, but at runtime the style property would contain an{ isExpression, value }wrapper object instead of a raw value.Is the intent that style properties can also be expression-wrapped at the migration level? If so, the
CompanionFeedbackButtonStyleResulttype (or thestylefield onCompanionMigrationFeedback) may need updating. If not, you'd want to unwrap the expression value before assigning.#!/bin/bash # Check what CompanionFeedbackButtonStyleResult looks like and how style is consumed ast-grep --pattern 'interface CompanionButtonStyleProps { $$$ }'packages/companion-module-base/src/module-api/input.ts (1)
383-418:⚠️ Potential issue | 🟡 Minor
CompanionInputFieldBonjourDeviceandCompanionInputFieldSecretcorrectly extend the generic base.Both pick up the
TKeygeneric parameter as expected. The JSDoc onCompanionInputFieldBonjourDevicestill mentionsisVisible(line 372) which has been removed in this PR — you might want to update it to referenceisVisibleExpressioninstead.📝 Suggested doc update
- * You must make sure to handle this, we recommend using the `isVisible` function to hide the manual input fields when a bonjour device is selected. + * You must make sure to handle this, we recommend using `isVisibleExpression` to hide the manual input fields when a bonjour device is selected.
🧹 Nitpick comments (22)
packages/companion-module-base/src/helpers/__tests__/tcp.spec.ts (1)
10-232: Consider cleaning up the commented-out tests when you get a chance.Nearly the entire test file is commented out (error propagation, listening, send, etc.), leaving only one active test case (
construct > ok). It looks like these were disabled during the API rework. It might be nice to either restore them adapted to the new API, or remove them and track re-enablement in a follow-up issue — whichever works best for you. Stale commented-out tests can get confusing over time.No rush on this — just something to keep in mind as the 2.0 work solidifies! 😊
packages/companion-module-host/src/versions.ts (1)
1-9: Looks good — tight version pinning makes sense for a major API transition.One small thought: it might be worth adding a brief comment on the expected format (semver) for
minVersion/maxVersion, just so future contributors know what's expected when they add entries. Totally optional though!vitest.config.mts (1)
1-14: Consider importing fromvitest/configdirectly — a nice polish!Since this is a dedicated Vitest config file, the official Vitest docs recommend importing
defineConfigfromvitest/config. This drops the triple-slash reference and eslint-disable, making the config cleaner while following Vitest's recommended pattern.✨ Optional simplification
-/// <reference types="vitest/config" /> -// eslint-disable-next-line n/no-extraneous-import -import { defineConfig } from 'vite' +import { defineConfig } from 'vitest/config'packages/companion-module-base/tools/schema-compile.mjs (1)
1-23: Nice script — clean and well-structured! 👍Just a tiny grammar nit on Line 13:
"The generated code be in esm export format"reads like it's missing a word — maybe"The generated code will be in ESM export format"?Otherwise, the approach of generating a standalone AJV validator and bundling it with esbuild to avoid runtime AJV dependencies for consumers is really solid. Thanks for this!
packages/companion-module-base/src/logging.ts (2)
19-22: Consider routing to the appropriate console method in the default sinkThe default sink routes everything through
console.log, which means errors and warnings won't show up with the expected severity in typical console/log-aggregation environments. Since this serves as the fallback (and is noted for unit testing), it's not critical — but a small tweak would make the default behavior more useful during development too.Suggested tweak
const defaultLoggingSink: LoggingSink = (source, level, message) => { // Default logging implementation for unit testing purposes, this should be replaced by the host application - console.log(`[${level.toUpperCase()}]${source ? ` [${source}]` : ''} ${message}`) + const prefix = `[${level.toUpperCase()}]${source ? ` [${source}]` : ''}` + switch (level) { + case 'error': + console.error(`${prefix} ${message}`) + break + case 'warn': + console.warn(`${prefix} ${message}`) + break + case 'debug': + console.debug(`${prefix} ${message}`) + break + default: + console.log(`${prefix} ${message}`) + break + } }
24-27: Minor: preferglobalThisoverglobalfor ESM consistencySince the project is moving to ESM,
globalThisis the standard cross-environment way to access the global object.globalis Node-specific and, while it works fine here,globalThiswould be more idiomatic with the ESM migration. Totally optional — just a small alignment thing! 🙂Suggested tweak
function logToSink(source: string | undefined, level: LogLevel, message: string) { - const sink = typeof global.COMPANION_LOGGER === 'function' ? global.COMPANION_LOGGER : defaultLoggingSink + const sink = typeof globalThis.COMPANION_LOGGER === 'function' ? globalThis.COMPANION_LOGGER : defaultLoggingSink sink(source, level, message) }packages/companion-module-base/src/module-api/__tests__/shared-udp-socket.spec.ts (1)
16-29: Default mock implementations throw synchronously rather than returning rejected promisesHey, just a small note — the default mock implementations (e.g., Line 20) throw synchronously, but the types expect these functions to return
Promise<...>. If an unmocked call slips through, it would throw a synchronous exception rather than a rejected promise, which could manifest differently depending on the caller's error handling.Since every test properly sets up
mockImplementationOncebefore triggering the mock, this won't cause issues in practice. But if you want the defaults to be more robust safety nets, you could make them async or returnPromise.reject(...):Suggested tweak (optional)
- sharedUdpSocketJoin: vi.fn<InstanceSharedUdpSocketContext['sharedUdpSocketJoin']>(() => { - throw new Error('Not implemented') + sharedUdpSocketJoin: vi.fn<InstanceSharedUdpSocketContext['sharedUdpSocketJoin']>(() => { + return Promise.reject(new Error('Not implemented')) }), - sharedUdpSocketLeave: vi.fn<InstanceSharedUdpSocketContext['sharedUdpSocketLeave']>(() => { - throw new Error('Not implemented') + sharedUdpSocketLeave: vi.fn<InstanceSharedUdpSocketContext['sharedUdpSocketLeave']>(() => { + return Promise.reject(new Error('Not implemented')) }), - sharedUdpSocketSend: vi.fn<InstanceSharedUdpSocketContext['sharedUdpSocketSend']>(() => { - throw new Error('Not implemented') + sharedUdpSocketSend: vi.fn<InstanceSharedUdpSocketContext['sharedUdpSocketSend']>(() => { + return Promise.reject(new Error('Not implemented')) }),.github/workflows/node.yaml (1)
65-68: Tiny nit: theifcondition is now always true.Since the matrix only contains
22.x, theif: matrix.node-version == '22.x'check on line 66 will always pass. Not harmful at all — just a leftover from the multi-version matrix. Feel free to keep it as a reminder for when you add more versions, or drop it to declutter. Totally up to you!packages/companion-module-host/src/internal/presets.ts (1)
24-28: Consider including the preset ID in validation warning messages for easier debugging. 🔍Currently, the validation warnings (lines 41, 83, 85) collect
presetName— which comes frompreset.nameand falls back to'Unknown'. Since preset names aren't guaranteed to be unique (and could all be'Unknown'), it might be more helpful to include the preset key (_id) in the messages so developers can quickly locate the problematic preset definition.A small tweak like
\${_id} (${presetName})`` would give both the lookup key and the human-readable name.💡 Suggested tweak
- for (const [_id, preset] of Object.entries(presets)) { + for (const [id, preset] of Object.entries(presets)) { if (!preset) continue if (preset.type !== 'simple') continue - const presetName = typeof preset.name === 'string' ? preset.name : 'Unknown' + const presetLabel = typeof preset.name === 'string' ? `${id} (${preset.name})` : idAnd then use
presetLabelin place ofpresetNamefor the push calls.packages/companion-module-host/src/internal/feedback.ts (1)
149-152: Tiny nit:feedbackis just re-assigned fromfeedback0without transformation.Line 152 (
const feedback = feedback0) looks like a leftover from a refactor. Feel free to usefeedback0directly or just name itfeedbackfrom the start — totally up to you!✨ Minor simplification
- const feedback0 = this.#feedbackInstances.get(id) - if (!feedback0) return - - const feedback = feedback0 + const feedback = this.#feedbackInstances.get(id) + if (!feedback) returnpackages/companion-module-host/src/internal/actions.ts (1)
172-172: Tiny nit:setActionDefinitionsis missing thepublickeyword.All other methods in this class explicitly declare
publicvisibility. This one works the same (default is public in TS), but adding the keyword would keep things consistent. Totally optional! 😊packages/companion-module-host/src/internal/__tests__/upgrade.spec.ts (2)
268-268: Tiny grammar nit in test name: "an actions" → "an action".Super minor, but
'an actions to upgrade'should probably be'an action to upgrade'since it's singular (one action at an earlier upgrade index). 😊
139-148: Inconsistent use ofanyvsJsonObjectin test type parameters.Some assertions use
literal<CompanionStaticUpgradeProps<any, any>>(Lines 142, 188, 224) while others useliteral<CompanionStaticUpgradeResult<JsonObject, JsonObject>>(Lines 115, 234, 295, 304). UsingJsonObjectconsistently would better match the actual generic constraints and keep things uniform. Totally optional — the tests work either way since these are assertion helpers!Also applies to: 159-159, 185-194, 222-230
.github/workflows/publish.yml (1)
148-153:cd ../..is fragile — it assumes exactly two levels of nesting.This works for the current
packages/companion-module-*layout, but if the structure ever changes, this will silently break. A safer approach would be tocdback to$GITHUB_WORKSPACEor to usepushd/popd.💡 Suggested alternative
- cd ${{ needs.determine-package.outputs.package-path }} OLD_VERSION=$(node -p "require('./package.json').version") - yarn version ${OLD_VERSION}-${{ needs.determine-package.outputs.prerelease }} - cd ../.. + cd ${{ needs.determine-package.outputs.package-path }} && \ + OLD_VERSION=$(node -p "require('./package.json').version") && \ + yarn version ${OLD_VERSION}-${{ needs.determine-package.outputs.prerelease }} + cd $GITHUB_WORKSPACEpackages/companion-module-host/src/internal/__tests__/feedback.spec.ts (1)
2-2: Deep relative import reaching into another package's__mocks__directory.
import { runAllTimers } from '../../../../companion-module-base/src/__mocks__/util.js'is a fragile cross-package path. If the directory structure changes, this breaks silently. Consider moving shared test utilities to a common location or exporting them via a test-utils entrypoint. No rush on this — just something to keep in mind for future cleanups! 🧹packages/companion-module-base/package.json (1)
8-25: Consider adding"types"conditions in the exports map for TypeScript consumers.TypeScript will generate
.d.tsfiles during the build (tsc --build tsconfig.jsonwithdeclaration: true), so it's worth explicitly mapping them in the exports. This ensures reliable type resolution, especially for nested exports like./manifestand./host-apiwhen consumers usemoduleResolution: "node16"or newer.💡 Example addition
"exports": { ".": { + "types": "./dist/main.d.ts", "import": "./dist/main.js", "require": "./dist/main.js" }, "./manifest": { + "types": "./dist/manifest.d.ts", "import": "./dist/manifest.js", "require": "./dist/manifest.js" }, "./host-api": { + "types": "./dist/host-api/main.d.ts", "import": "./dist/host-api/main.js", "require": "./dist/host-api/main.js" },packages/companion-module-base/src/module-api/action.ts (1)
105-112: Naming nit:Tschemas→TSchemasfor TypeScript convention consistency.The generic parameter
Tschemasuses a lowercases, which is a bit unusual for TypeScript generics where PascalCase after theTprefix is the norm (e.g.,TOptions,TConfig). Totally minor, but since this is a public API type that module authors will interact with, consistent naming could help readability.✏️ Suggested rename
-export type CompanionActionDefinitions< - Tschemas extends Record<string, CompanionActionSchema<CompanionOptionValues>> = Record< - string, - CompanionActionSchema<CompanionOptionValues> - >, -> = { - [K in keyof Tschemas]: CompanionActionDefinition<Tschemas[K]['options']> | false | undefined -} +export type CompanionActionDefinitions< + TSchemas extends Record<string, CompanionActionSchema<CompanionOptionValues>> = Record< + string, + CompanionActionSchema<CompanionOptionValues> + >, +> = { + [K in keyof TSchemas]: CompanionActionDefinition<TSchemas[K]['options']> | false | undefined +}packages/companion-module-host/src/instance.ts (2)
38-39: Consider typing the initial config/secrets defaults more safely.The
{} as anycasts are understandable as temporary placeholders beforeinit()sets the real values, but if any code path accidentally accesses config beforeinit()completes, it could lead to subtle bugs. A small comment noting these are overwritten duringinit()would help clarify intent.
375-390: UDP socket message routing iterates all handlers — consider a break after match.If
handleIdis unique per socket (which seems likely given the name), you couldbreakorreturnafter finding the matching handler to skip unnecessary iterations. Not a big deal for small handler sets, but a nice micro-optimization.✏️ Suggested optimization
async sharedUdpSocketMessage(msg: SharedUdpSocketMessage): Promise<void> { for (const socket of this.#instanceContext.sharedUdpSocketHandlers.values()) { if (socket.handleId === msg.handleId) { socket.receiveSocketMessage(msg) + break } } } async sharedUdpSocketError(msg: SharedUdpSocketError): Promise<void> { for (const socket of this.#instanceContext.sharedUdpSocketHandlers.values()) { if (socket.handleId === msg.handleId) { const error = new Error(msg.errorMessage) error.stack = msg.errorStack socket.receiveSocketError(error) + break } } }packages/companion-module-host/src/context.ts (1)
54-79: TheTODO module-lib - versioned types?comments on lines 59 and 73 are worth tracking.These hint at a future need to version the input field types sent across the host boundary. Might be worth opening an issue to track this if it's not already tracked, so it doesn't get lost.
Would you like me to open an issue to track the versioned types consideration for
HostActionDefinition.optionsandHostFeedbackDefinition.options?packages/companion-module-base/src/module-api/base.ts (2)
286-303:assertNeverdoesn't throw — theinfofallback is the actual safety net here.Looking at the
assertNeverimplementation (function assertNever(_val: never): void { // Nothing to do }), it silently accepts the value without throwing. So thethis.#logger.info(message)on line 301 is actually reachable as a runtime fallback if a newLogLevelis added without updating this switch.This works, but the pattern is a bit unusual — typically
assertNeverthrows to surface the unhandled case during development. Since this one is silent, you're relying on the TypeScript compiler error alone to catch missing cases. If you're happy with that, maybe a brief comment on line 300-301 explaining it's an intentional runtime fallback would help future readers.
81-86: The double validation check is a bit redundant but harmless.
isInstanceContext<TManifest>(internal)already verifiesobj2._isInstanceContext === true, so the additional!internal._isInstanceContexton line 82 is checking the same thing. Not a problem at all — just a minor observation!
packages/companion-module-base/src/module-api/preset/definition.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/companion-module-base/assets/manifest.schema.json (1)
46-46:⚠️ Potential issue | 🟡 MinorTiny typo in an existing line: "maintiners" → "maintainers".
I know this line isn't part of your changes, so totally fine to defer — but since you're touching this file anyway, might be a nice drive-by fix if you feel like it. 😊
✏️ Optional typo fix
- "description": "List of active maintiners", + "description": "List of active maintainers",packages/companion-module-base/src/module-api/input.ts (1)
242-279:⚠️ Potential issue | 🟡 MinorThe JSDoc example for
multidropdowndoesn't match the actualdefaulttype.The example on Line 257 shows
default: 'bob'(a single string), but the interface on Line 270 definesdefault: TChoiceId[](an array). This looks like a copy-paste from the single-select dropdown example. Module developers following this example would hit a type error.📝 Suggested fix for the example
* default: 'bob' + * default: ['bob'](Line 257)
packages/companion-module-base/src/module-api/upgrade.ts (1)
169-175:⚠️ Potential issue | 🟡 MinorConsider how
ExpressionOrValueobjects should be handled when assigning to style properties.On line 171,
rawValisExpressionOrValue<JsonValue | undefined>(an object like{ value: ..., isExpression: boolean }), butCompanionButtonStylePropsexpects plain values (e.g.,color: number,bgcolor: string,text: string) with onlytextExpression?: booleanas a separate flag.The
as anycast bypasses this type mismatch. While this works for migrations, it's worth confirming whether:
- The style system is actually designed to accept
ExpressionOrValueobjects, or- These should be unwrapped first (e.g., extracting
rawVal.valuefor plain properties and settingtextExpressionwhen appropriate)The TODO comment in
companion-module-host/src/internal/upgrade.ts("TODO - style?") suggests this behavior may not have been fully validated. A quick check of how the resulting style objects are used downstream would help clarify the safest approach.
🧹 Nitpick comments (3)
packages/companion-module-base/assets/manifest.schema.json (1)
204-209: Consider adding a"default"keyword toaddressFamilyfor better tooling support.The description nicely says it defaults to
"ipv4"when not specified, which is great. A small optional improvement: adding an explicit"default": "ipv4"to the schema would let JSON Schema-aware tooling (editors, doc generators, form builders) surface that default automatically. Totally fine to skip if the default is enforced purely at runtime — just a thought!💡 Optional schema enhancement
"addressFamily": { "type": "string", "description": "The address family to use when connecting to the device. If not specified, it will default to ipv4. This is useful for devices or modules that do not support ipv4 and ipv6, but discovery will find them on both", - "enum": ["ipv4", "ipv6", "ipv4+6"] + "enum": ["ipv4", "ipv6", "ipv4+6"], + "default": "ipv4" }.github/workflows/publish.yml (2)
148-152: Small note:cd ../..assumes a fixed directory depth.The
cd ../..on line 152 works correctly today since package paths are all two levels deep (packages/companion-module-*), but it's a bit fragile if paths ever change. A minor thing — just flagging it. You could usecd $GITHUB_WORKSPACEorcd ${{ github.workspace }}instead for a more resilient approach, but totally optional!💡 Optional alternative
cd ${{ needs.determine-package.outputs.package-path }} OLD_VERSION=$(node -p "require('./package.json').version") yarn version ${OLD_VERSION}-${{ needs.determine-package.outputs.prerelease }} - cd ../.. + cd ${{ github.workspace }}
34-34: Node version matrix is now a single entry — consider simplifying if you don't plan to add more.The
strategy.matrixwith[22.x]is technically fine and makes it easy to add versions later. Just noting that if you don't anticipate adding more versions soon, you could simplify by removing the matrix entirely. But it's also totally reasonable to keep it for future flexibility — your call!
Since splitting out the upgrade methods in 1.12, these properties serve no real purpose as there is no reason to tell the module about disabled entities outside of upgrades. also some review comments
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)
packages/companion-module-base/src/module-api/input.ts (1)
219-240:⚠️ Potential issue | 🟡 MinorMinor type-safety consideration with
allowCustom+ typedTChoiceIdHey, just a small thing to flag — when
allowCustomistrue, the JSDoc on line 232 correctly notes that custom values always come through as strings. But if a module developer setsTChoiceIdtonumber, the types would suggest the value is always a number, while custom entries will actually be strings at runtime.This might be fine if the intent is for developers to handle that coercion themselves (and the note is there), but it could be a subtle footgun. Something to consider down the road — maybe a conditional type or an overload that widens to
stringwhenallowCustomis set? Totally not blocking though!
🧹 Nitpick comments (7)
packages/companion-module-base/assets/manifest.schema.json (1)
204-209: Consider adding a"default": "ipv4"toaddressFamily.The description says "If not specified, it will default to ipv4" — which is helpful prose, but adding an explicit
"default"keyword would let schema-aware tooling (code generators, UI form builders, validators) pick up that default automatically. Totally optional, but it'd make the schema a bit more self-describing.💡 Suggested tweak
"addressFamily": { "type": "string", "description": "The address family to use when connecting to the device. If not specified, it will default to ipv4. This is useful for devices or modules that do not support ipv4 and ipv6, but discovery will find them on both", - "enum": ["ipv4", "ipv6", "ipv4+6"] + "enum": ["ipv4", "ipv6", "ipv4+6"], + "default": "ipv4" }.github/workflows/publish.yml (2)
97-104: Minor: prerelease version suffix always saysnightly-even forexperimentalbuilds.Hey, just noticed a small naming inconsistency here —
PRERELEASE_TAGis hardcoded with anightly-prefix on line 100, but when the branch isn'tmain,TAGis set toexperimental(line 94). So you'd end up with a version like1.0.0-nightly-feat-something-...-hashpublished under theexperimentalnpm tag.Not a functional problem at all, but it could be a little confusing when inspecting published versions. You might consider using
$TAGin the prefix instead:💡 Optional tweak
- PRERELEASE_TAG=nightly-$(echo "${{ github.ref_name }}" | sed -r 's/[^a-z0-9]+/-/gi') + PRERELEASE_TAG=${TAG}-$(echo "${{ github.ref_name }}" | sed -r 's/[^a-z0-9]+/-/gi')
148-153: Nit:cd ../..is a tad fragile — consider returning to workspace root more robustly.This works perfectly today since both package paths are two directories deep (
packages/companion-module-*), but if the structure ever changes, this could silently break. A small defensive tweak:💡 Optional tweak
cd ${{ needs.determine-package.outputs.package-path }} OLD_VERSION=$(node -p "require('./package.json').version") yarn version ${OLD_VERSION}-${{ needs.determine-package.outputs.prerelease }} - cd ../.. + cd ${{ github.workspace }}packages/companion-module-base/src/module-api/preset/definition.ts (1)
81-103: Mixing named properties with a numeric index signature — worth a quick thoughtThe
CompanionButtonStepActionsinterface uses a numeric index signature ([duration: number]) alongside named string properties (down,up,name, etc.) to represent long-press duration groups. This is valid TypeScript, but it's a somewhat unusual pattern — consumers iterating over the object's keys will need to carefully distinguish named properties from duration-keyed entries.If you've already considered this and prefer the ergonomics of
step[1000]over something likestep.durations[1000], that's totally fair! Just wanted to flag it as something that could trip up module authors who might not expect the mixed shape. A brief JSDoc note on the interface explaining that numeric keys represent durations could go a long way.packages/companion-module-host/src/internal/feedback.ts (2)
141-158: Minor: redundant variable reassignment on line 152.
const feedback = feedback0doesn't add anything here —feedback0could just be namedfeedbackdirectly on line 149. Totally a nit though, no pressure to change!✨ Tiny cleanup
- const feedback0 = this.#feedbackInstances.get(id) - if (!feedback0) return - - const feedback = feedback0 + const feedback = this.#feedbackInstances.get(id) + if (!feedback) return
326-337:checkFeedbacks— minor thought on empty-set semantics.When
typesis empty (feedbackTypesis[]),types.size === 0is true, so the conditiontypes.size === 0 || types.has(feedback.feedbackId)means all feedbacks are rechecked. This is correct behavior and matches the test expectations — just calling it out since the "empty means all" convention can be surprising to future readers. A brief inline comment could help, but totally optional!packages/companion-module-host/src/internal/__tests__/feedback.spec.ts (1)
249-296: Sharedmanagerat describe scope — intentional and works, but worth noting.The
FeedbackManagerinstance and mocks are created atdescribescope (lines 250-252) and shared across tests. ThebeforeAllsets up initial state, andbeforeEachclears mocks. This is fine for these tests since they build on accumulated state, but it does mean test ordering matters. Just something to keep in mind if tests are ever reordered or new ones added!
packages/companion-module-base/src/module-api/preset/definition.ts
Outdated
Show resolved
Hide resolved
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)
packages/companion-module-base/assets/manifest.schema.json (1)
169-184:⚠️ Potential issue | 🟠 MajorHeads up:
typeisn't in therequiredarray — was that intentional?The commit message says "feat!: require type property in manifest," and the new
typeproperty is defined (lines 9–13) withenum: ["connection"], but it's not listed inrequired. If the intent is to make it mandatory, it looks like it just needs to be added here. Easy to miss with a list this long! 😄Suggested fix
"required": [ + "type", "id", "name", "shortname",
🧹 Nitpick comments (5)
packages/companion-module-base/src/module-api/preset/definition.ts (1)
81-103: Numeric index signature for duration groups — worth a brief noteThe
[duration: number]index signature is a clever way to model duration-keyed long-press groups alongside the nameddown/up/rotate_*properties. Just a friendly heads-up: because TypeScript won't prevent accessing something likestep[0](which returns the union type rather than an error), consumers could accidentally use0as a duration key without realizing it overlaps conceptually with thedownactions. If that's intentional and documented elsewhere, no worries at all — just wanted to flag it in case it helps!packages/companion-module-host/src/internal/__tests__/feedback.spec.ts (2)
2-2: Cross-package deep import into__mocks__— consider a shared test utilityThis reaches across package boundaries into
companion-module-base/src/__mocks__/util.js. It works fine in a monorepo, but if the mock ever moves or the package structure changes, this breaks silently. Totally up to you whether to extract a shared test-utils workspace package or keep it as-is for now — just flagging it as something to keep in mind! 🙂
247-250: Shared mutable state across tests in thisdescribeblockJust something to be aware of —
manager,mockSetFeedbackDefinitions, andmockUpdateFeedbackValuesare created once at thedescribescope and shared across all tests. This means tests depend on execution order (thebeforeAllsetup). It works fine here sincebeforeEachclears mock call counts, and the tests are read-only checks. If future tests start mutating manager state though, this could get tricky. No action needed now, just a note for future you! 😊packages/companion-module-host/src/context.ts (2)
90-95: When wouldfeedbackTypebeundefined?Just curious — on line 93,
feedbackTypeis typed asHostFeedbackType | undefined. I'm wondering what scenario leads to an undefined feedback type when reporting a value. If it's for cases where the feedback definition was removed but a value is still being flushed, a quick inline comment would help future readers understand the intent. No worries if it's obvious in context though! 😄
16-20: Consider usingimport typefor interface-only importsLines 1–15 correctly use
import typefor type-only imports, but lines 16–20 importSharedUdpSocketMessageJoin,SharedUdpSocketMessageLeave, andSharedUdpSocketMessageSendwith a regularimport. Since these are all interfaces and are only used in type annotations, usingimport typewould maintain consistency and is a best practice that helps clarify the intent.✏️ Suggested fix
-import { +import type { SharedUdpSocketMessageJoin, SharedUdpSocketMessageLeave, SharedUdpSocketMessageSend, } from '@companion-module/base/host-api'
| * Keywords for the preset | ||
| * This is extra search terms to allow users to find the right preset | ||
| */ | ||
| keywords?: string[] |
There was a problem hiding this comment.
Tiny grammar nit in JSDoc
Line 34: "This is extra search terms" reads a bit oddly — "These are" would be more natural. Totally minor, just figured I'd mention it while looking through! 😊
Suggested tweak
- * This is extra search terms to allow users to find the right preset
+ * These are extra search terms to allow users to find the right preset📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * Keywords for the preset | |
| * This is extra search terms to allow users to find the right preset | |
| */ | |
| keywords?: string[] | |
| * Keywords for the preset | |
| * These are extra search terms to allow users to find the right preset | |
| */ | |
| keywords?: string[] |
| export interface HostActionDefinition { | ||
| id: string | ||
| name: string | ||
| sortName: string | undefined | ||
| description: string | undefined | ||
| options: SomeCompanionActionInputField[] // TODO module-lib - versioned types? | ||
| optionsToMonitorForSubscribe: string[] | undefined | ||
| hasLearn: boolean | ||
| learnTimeout: number | undefined | ||
| hasLifecycleFunctions: boolean | ||
| } | ||
|
|
||
| export type HostFeedbackType = 'boolean' | 'value' | 'advanced' | ||
|
|
||
| export interface HostFeedbackDefinition { | ||
| id: string | ||
| name: string | ||
| sortName: string | undefined | ||
| description: string | undefined | ||
| options: SomeCompanionFeedbackInputField[] // TODO module-lib - versioned types? | ||
| type: HostFeedbackType | ||
| defaultStyle?: CompanionFeedbackButtonStyleResult | ||
| hasLearn: boolean | ||
| showInvert: boolean | undefined | ||
| learnTimeout: number | undefined | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for 'value' feedback type in test files
echo "=== Searching for 'value' feedback type in tests ==="
rg -n "value" --type=ts -g '*test*' -g '*spec*' | grep -i "feedback\|type" | head -20
echo ""
echo "=== Searching more broadly for HostFeedbackType tests ==="
rg -n "HostFeedbackType" --type=ts -g '*test*' -g '*spec*'
echo ""
echo "=== Finding test files ==="
fd -e test.ts -e spec.ts | head -10Repository: bitfocus/companion-module-base
Length of output: 2758
🏁 Script executed:
#!/bin/bash
# Get the feedback test file and look for feedback type definitions
cat -n packages/companion-module-host/src/internal/__tests__/feedback.spec.ts | head -100
echo ""
echo "=== Searching for feedback type definitions in test ==="
rg -n "type\s*:" packages/companion-module-host/src/internal/__tests__/feedback.spec.ts -A 1 -B 1Repository: bitfocus/companion-module-base
Length of output: 5582
🏁 Script executed:
#!/bin/bash
echo "=== Searching for 'value' feedback type in implementation (non-test files) ==="
rg -n "type.*['\"]value['\"]" --type=ts -v -g '*test*' -g '*spec*' | head -20
echo ""
echo "=== Checking CompanionFeedbackDefinition for 'value' type ==="
rg -n "CompanionFeedbackDefinition\|type.*value" packages/companion-module-base/src -A 5 -B 5 | head -50
echo ""
echo "=== Looking for feedback type definitions in base package ==="
fd -e ts packages/companion-module-base/src | xargs rg -l "type.*'boolean'.*'advanced'" | head -5Repository: bitfocus/companion-module-base
Length of output: 1796
🏁 Script executed:
#!/bin/bash
echo "=== Looking at context.ts around HostFeedbackType definition ==="
rg -n "HostFeedbackType" packages/companion-module-host/src/context.ts -B 5 -A 10
echo ""
echo "=== Checking feedback.ts in module-api ==="
rg -n "value\|HostFeedbackType\|type.*:" packages/companion-module-base/src/module-api/feedback.ts | head -30
echo ""
echo "=== Checking feedback.ts in internal ==="
rg -n "type.*value\|HostFeedbackType" packages/companion-module-host/src/internal/feedback.ts -B 2 -A 2 | head -50Repository: bitfocus/companion-module-base
Length of output: 1258
🏁 Script executed:
#!/bin/bash
echo "=== Reading module-api/feedback.ts ==="
cat -n packages/companion-module-base/src/module-api/feedback.ts | head -150Repository: bitfocus/companion-module-base
Length of output: 6422
The 'value' feedback type deserves test coverage
Great work on the type definitions! I noticed that 'value' is a fully supported feedback type (it's defined in CompanionFeedbackDefinitionBase in the module-api), but the feedback tests only cover 'boolean' and 'advanced'. Would be nice to add a test case for 'value' feedbacks when you have a moment—no pressure though!
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/companion-module-base/src/helpers/__tests__/udp.spec.ts (1)
348-358:⚠️ Potential issue | 🟡 MinorSame shared-state issue exists here in the
sendAsync'destroyed' testSame concern as in the "send (sync)" block —
mySocket.destroy()callsrawSocket.removeAllListeners()on the shared socket. Worth using an independentMinimalSockethere too for the same reasons.💡 Suggested fix
it('destroyed', async () => { - createSocketMock.mockReturnValueOnce(rawSocket as any) + const destroySocket = new MinimalSocket() + destroySocket.bind.mockImplementation(() => destroySocket as any) + createSocketMock.mockReturnValueOnce(destroySocket as any) const mySocket = new UDPHelper('1.2.3.4', 852) expect(mySocket).toBeTruthy() mySocket.destroy() - expect(rawSocket.close).toHaveBeenCalledTimes(1) + expect(destroySocket.close).toHaveBeenCalledTimes(1) await expect(mySocket.sendAsync('test')).rejects.toThrow('Cannot write to destroyed socket') })
🧹 Nitpick comments (8)
packages/companion-module-base/src/helpers/udp.ts (3)
235-246: Consider guarding against double-destroy()callsIf
destroy()is called twice,this.#socket.close()will be invoked on an already-closed socket, which throws in Node's dgram. Since#destroyedis already tracked, a simple early return could make this safe. Totally up to you — just a defensive measure that can save headaches in cleanup paths (e.g.,finallyblocks stacking with explicit destroy calls).💡 Possible guard
destroy(): void { + if (this.#destroyed) return + this.#destroyed = true if (this.#missingErrorHandlerTimer !== undefined) {
112-113: Optional: clean up commented-out code blocksThere are a few commented-out code blocks (
this.bound,pending_memberships,addMembership) that look like remnants from an earlier implementation. If these aren't planned for re-inclusion, removing them would tidy things up. No rush — just something to consider when you're polishing.Also applies to: 139-145, 221-227
248-254: Nit: loose equality in#new_statusLine 250 uses
!=— TypeScript won't complain since both sides are the same type, but!==is more idiomatic and avoids any future surprises if the type ever changes. Totally minor!💡 Suggested tweak
- if (this.#lastStatus != status) { + if (this.#lastStatus !== status) {packages/companion-module-base/src/helpers/__tests__/udp.spec.ts (1)
277-281: Nit: mock parameter names are misleadingThe
rawSocket.send.mockImplementation((msg, offset, length, cb) => {...})parameter names suggest the dgramsend(msg, offset, length, port, address, callback)overload, butUDPHelperactually callssocket.send(message, port, host, callback). So positionally,offsetis actually the port andlengthis the host.Renaming to
(_msg, _port, _host, cb)would make the mock's intent clearer for anyone reading these tests later. Totally optional — just a readability thing!Also applies to: 290-294, 307-312, 367-371, 380-384, 394-398
packages/companion-module-base/src/helpers/tcp.ts (2)
198-205: Consider guarding against callingconnect()while already connected.
connect()checks for#destroyedand#connecting, but not#connected. If a caller invokesconnect()on an already-connected socket, it would call_socket.connect()again, which can lead to unexpected behavior from the underlyingnet.Socket. Since this is a public method, a small guard could prevent accidental misuse.💡 Suggested guard
connect(): boolean { if (this.#destroyed) throw new Error('Cannot connect destroyed socket') + if (this.#connected) return false if (this.#connecting) return false this.#connecting = true this._socket.connect(this.#port, this.#host) return true }
320-326: Tiny nit: prefer!==over!=for the status comparison.Both work identically here since you're comparing string enums and
undefined, but!==is more idiomatic in TypeScript and avoids potential lint warnings. Totally optional though!✏️ Suggested tweak
- if (this.#lastStatus != status) { + if (this.#lastStatus !== status) {packages/companion-module-base/src/helpers/__tests__/tcp.spec.ts (2)
29-33: Minor: thesetKeepAliveassertion includes a trailingundefined.The mock captures all arguments including unset optional ones, so
['setKeepAlive', true, undefined]works — but it's slightly coupled to the mock's implementation detail of recording all parameters. Not a problem, just something to be aware of if the mock changes. Totally fine as-is!
163-267: You might want to add tests for the write-error path — both forsend()andsendAsync().The current tests cover validation and happy-path sending beautifully, but there's no coverage for what happens when
_socket.writecalls back with an error. Forsend(), you'd want to verify that the'error'event is emitted andisConnectedflips tofalse. ForsendAsync(), you'd want to verify that the promise rejects (and'error'is not emitted). Since the mock'swritealways succeeds, you could configure it viaonWriteto simulate a failure.No pressure to add this now, but it would really round out the test suite for the new error-handling semantics that this PR introduces! 🧪
| it('destroyed', () => { | ||
| createSocketMock.mockReturnValueOnce(rawSocket as any) | ||
|
|
||
| const mySocket = new UDPHelper('1.2.3.4', 852) | ||
| expect(mySocket).toBeTruthy() | ||
|
|
||
| mySocket.destroy() | ||
| expect(rawSocket.close).toHaveBeenCalledTimes(1) | ||
|
|
||
| expect(() => mySocket.send('test')).toThrow('Cannot write to destroyed socket') | ||
| }) |
There was a problem hiding this comment.
Heads-up: this test silently breaks the shared rawSocket for subsequent tests
The 'destroyed' test creates a new UDPHelper (mySocket) that receives the same rawSocket instance used by the shared socket from beforeAll. When mySocket.destroy() runs, it calls rawSocket.removeAllListeners() (via UDPHelper.destroy()), which strips all event listeners — including those wired up by the shared socket.
Current tests after this one only exercise the send callback path (a closure, not an event listener), so they still pass. But if someone later adds a test in this block that relies on rawSocket events (e.g., emitting 'error' or 'message' on rawSocket and expecting the shared socket to react), it'll silently fail.
The fix is straightforward — give the 'destroyed' test its own independent MinimalSocket:
💡 Suggested fix
it('destroyed', () => {
- createSocketMock.mockReturnValueOnce(rawSocket as any)
+ const destroySocket = new MinimalSocket()
+ destroySocket.bind.mockImplementation(() => destroySocket as any)
+ createSocketMock.mockReturnValueOnce(destroySocket as any)
const mySocket = new UDPHelper('1.2.3.4', 852)
expect(mySocket).toBeTruthy()
mySocket.destroy()
- expect(rawSocket.close).toHaveBeenCalledTimes(1)
+ expect(destroySocket.close).toHaveBeenCalledTimes(1)
expect(() => mySocket.send('test')).toThrow('Cannot write to destroyed socket')
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('destroyed', () => { | |
| createSocketMock.mockReturnValueOnce(rawSocket as any) | |
| const mySocket = new UDPHelper('1.2.3.4', 852) | |
| expect(mySocket).toBeTruthy() | |
| mySocket.destroy() | |
| expect(rawSocket.close).toHaveBeenCalledTimes(1) | |
| expect(() => mySocket.send('test')).toThrow('Cannot write to destroyed socket') | |
| }) | |
| it('destroyed', () => { | |
| const destroySocket = new MinimalSocket() | |
| destroySocket.bind.mockImplementation(() => destroySocket as any) | |
| createSocketMock.mockReturnValueOnce(destroySocket as any) | |
| const mySocket = new UDPHelper('1.2.3.4', 852) | |
| expect(mySocket).toBeTruthy() | |
| mySocket.destroy() | |
| expect(destroySocket.close).toHaveBeenCalledTimes(1) | |
| expect(() => mySocket.send('test')).toThrow('Cannot write to destroyed socket') | |
| }) |
…hecked calling `checkFeedbacks` with no arguments is no longer supported, and will produce an error. This is to discourage the old practise of calling it like this when any feedback has changed.
3 participants
I don't know when 2.0 of this api will happen (maybe companion 4.1?), this is some changes that should be included in that release
It needs some further testing, but the change to esm should have no impact for modules due to
require(esm)support being enabled by default since 22.12TODO:
decide on and include RFC: Reduce controlId awareness from modules #141decide whether to remove advanced-feedbacksI have been playing with this in the https://github.com/bitfocus/companion-surface-base api. This moves us to be a little more like 2.x, with the main file being a class export. And leaves the ipc wrapping to be the responsibility of Companion. Which means that we have scope to easily inject code into the modules, and avoid baking some code into every module which may be better to be provided by companion.
If we had this, then we could more easily decide to leave certain operations to be done inside the module thread, and minimise the penalty of ipc. doing that today is hard because we need to handle old modules too
Release notes drafting:
export default class ...(andexport const UpgradeScripts = [])moduleResolution?textfields which haveuseVariablesset. And for anything in expression mode.requiredproperty on input fields has been removed. Insteadtextinputhas gained aminLengthproperty (we were barely respecting this anyway, I think it was confusing in the old form)disableAutoExpression/ch/01/mixor0is not intuitive. Instead make that be something simple like1orch1(ideally with some fuzzy matching). Your users will thank you.Note: if you do this while updating to the 2.0 api, you can rely on the fact that no user values will be expressions. If you do it later, you will have to consider that these could be expressions, making fixing them up much harder
Summary by CodeRabbit
New Features
Breaking Changes
Documentation