update d.ts and .js files#1687
Closed
summeroff wants to merge 8 commits into
Closed
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds automation and documentation to keep the committed JavaScript/TypeScript declaration outputs under js/ in sync with their TypeScript sources (notably js/module.ts), and updates the JS build configuration accordingly.
Changes:
- Adds a GitHub Actions workflow + CI script to regenerate
js/outputs and fail when committed generated files are out of date. - Updates local build flow to run the JS generation step before building/installing native artifacts.
- Updates
js/tsconfig.json(target bump to ES2020 and removes a deprecated compiler option) and refreshes committed generated outputs (js/*.js,js/*.d.ts).
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents that js/module.ts is the source of truth and how/where generated outputs are enforced. |
| package.json | Chains build:javascript into local:build to reduce drift locally. |
| js/type_check.js | Updated committed tsc output to match current generation. |
| js/tsconfig.json | Updates TS emit target to ES2020 and removes suppressImplicitAnyIndexErrors. |
| js/module.js | Updated committed tsc output to match current generation. |
| js/module.d.ts | Updated committed declaration output to match current generation. |
| ci/check-js-generated.sh | New CI helper script to detect stale generated outputs after running yarn build:javascript. |
| .github/workflows/check-js-generated.yml | New workflow to run JS generation and invoke the stale-check script on PRs/pushes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ntain permissions' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
summeroff
force-pushed
the
js_tsc_build_pipeline
branch
from
051ab94 to
94f86aa
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
js/module.ts:744
IProperties.get()is typed as always returningIProperty, but the native implementation returnsundefinedwhen the named property is not found (Properties::Getinobs-studio-client/source/properties.cpp). The return type (and the JSDoc, which currently mentionsnull) should be updated toIProperty | undefinedto match runtime.
/**
* Obtains property matching name.
* @param name The name of the property to fetch.
* @returns - The property instance or null if not found
*/
get(name: string): IProperty;
Comment on lines
703
to
706
| @@ -734,6 +704,19 @@ | |||
| * Otherwise or if end of the list, returns false. | |||
| */ | |||
| next(): IProperty; | |||
Comment on lines
+708
to
+712
| /** | ||
| * Uses the current object to obtain the previous property in the list. | ||
| * Returns undefined when the current property is the first. | ||
| */ | ||
| previous(): IProperty; |
Comment on lines
+731
to
+735
| /** Obtains the first property in the list. */ | ||
| first(): IProperty; | ||
|
|
||
| /** Obtains the last property in the list. */ | ||
| last(): IProperty; |
Comment on lines
1297
to
1302
| /** | ||
| * Forward a serializable message to the underlying source plugin. | ||
| * Note: only registered on input sources on the native side; calling on | ||
| * a filter, scene, or transition will throw at runtime. | ||
| */ | ||
| sendMessage(message: ISettings): void; |
Comment on lines
1283
to
1296
| export interface ISource extends IConfigurable, IReleasable { | ||
| /** | ||
| * Send remove signal to other holders of the current reference. | ||
| */ | ||
| remove(): void; | ||
|
|
||
| /** | ||
| * Send a save signal to sources themselves. | ||
| * This should always be called before saving to disk | ||
| * as it allows the source to know it needs to update | ||
| * its settings. | ||
| */ | ||
| save(): void; | ||
|
|
Comment on lines
+1499
to
1502
| export interface IModuleFactory { | ||
| open(binPath: string, dataPath: string): IModule; | ||
| loadAll(): void; | ||
| addPath(path: string, dataPath: string): void; | ||
| logLoaded(): void; | ||
| modules(): String[]; | ||
| } |
Comment on lines
1504
to
+1506
| export interface IModule { | ||
| initialize(): void; | ||
| filename(): string; | ||
| name(): string; | ||
| author(): string; | ||
| description(): string; | ||
| binPath(): string; | ||
| dataPath(): string; | ||
| status(): number; | ||
| readonly fileName: string; |
const enum members are inlined at compile time and produce no runtime object. Downstream consumers that bundle with isolatedModules: true cannot inline across module boundaries, so osn.E*.* reads resolve to undefined at runtime. Dropping const makes tsc emit real runtime enum objects into module.js. Source change folded in from #1430 (Ava Creeth); the js/ outputs were regenerated via the build:javascript pipeline. type_check.js now resolves EPropertyType through the runtime object instead of inlined numeric literals, demonstrating the same inlining fix in this repo's own code. Co-authored-by: Ava Creeth <avacreeth@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
js/module.ts:745
IProperties.first(),last(), andget()returnundefinedat runtime when the list is empty or the property name isn’t found (see obs-studio-client/source/properties.cpp), but the typings currently claim they always returnIProperty. The JSDoc mentions returningnull, which also doesn’t match runtime (undefined).
/** Obtains the first property in the list. */
first(): IProperty;
/** Obtains the last property in the list. */
last(): IProperty;
count(): number;
/**
* Obtains property matching name.
* @param name The name of the property to fetch.
* @returns - The property instance or null if not found
*/
get(name: string): IProperty;
}
Comment on lines
+699
to
+712
| * Uses the current object to obtain the previous property in the list. | ||
| * Returns undefined when the current property is the first. | ||
| */ | ||
| previous(): IProperty; |
Comment on lines
1504
to
+1507
| export interface IModule { | ||
| initialize(): void; | ||
| filename(): string; | ||
| name(): string; | ||
| author(): string; | ||
| description(): string; | ||
| binPath(): string; | ||
| dataPath(): string; | ||
| status(): number; | ||
| readonly fileName: string; | ||
| readonly name: string; |
Comment on lines
+1499
to
1502
| export interface IModuleFactory { | ||
| open(binPath: string, dataPath: string): IModule; | ||
| loadAll(): void; | ||
| addPath(path: string, dataPath: string): void; | ||
| logLoaded(): void; | ||
| modules(): String[]; | ||
| } |
Comment on lines
+1
to
+8
| #!/bin/bash | ||
| # Verify that the committed tsc outputs under js/ match what regenerating | ||
| # from js/module.ts would produce. CI runs `yarn build:javascript` first, | ||
| # so any change under js/ at this point is a stale generated file. | ||
| dirty=$(git status --porcelain -- js/) | ||
|
|
||
| set +x | ||
| if [[ $dirty ]]; then |
CI: make ci/check-js-generated.sh fail-fast (set -euo pipefail) so a failing git invocation can no longer leave `dirty` empty and pass silently; drop the no-op `set +x`. module.ts typing accuracy (verified against obs-studio-client/source): - IProperty.next()/previous() and IProperties.first()/last()/get() return undefined at end-of-list / empty / not-found, so type them `IProperty | undefined`; fix the stale next() JSDoc and get() "null". - Move sendMessage() from ISource to IInput — only Input registers it natively (input.cpp); Filter/Scene/Transition do not. - Hoist load() to the ISource base (registered on Input/Filter/Scene/ Transition) and drop the per-interface duplicates; IFilter now gets it. - IModuleFactory.modules() returns primitive strings -> string[]. - IModule.initialize() returns a boolean natively, not void. Remove stray trailing `;` after enum/interface blocks so the generated module.js no longer emits empty statements. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comment on lines
+1284
to
+1287
| /** | ||
| * Re-trigger the source's load step on the server. | ||
| */ | ||
| load(): void; |
Restores `const enum` for all exported enums. The regular-enum conversion (folded in from #1430) made the enum runtime objects live in module.js, which broke the desktop webpack build: ~11 desktop files import enum values directly from 'obs-studio-node' and relied on const-enum inlining to keep them type-only. As regular enums those imports became runtime value imports, pulling obs-studio-node/module.js (and its native obs_studio_client.node require) into the renderer bundle, which has no node-loader and does not externalize osn. desktop never bundles osn (it loads it at runtime via window.require), so #1430's "bundlers can't inline const enums" rationale does not apply to the primary consumer. Keeping const enums. The js/ build pipeline, generated-file CI check, API-drift fixes, and the typing-accuracy fixes from PR review are unaffected and retained. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Make
js/module.tsthe single source of truth and treat the shipped JS surface as generated tsc output:js/module.{js,d.ts}and theindex.*/type_check.*siblings are tsc outputs ofjs/tsconfig.json. The npm package ships them as-is, so they stay committed, but they must matchtscoutput.yarn build:javascriptregenerates them;yarn local:buildchains it..github/workflows/check-js-generated.yml(viaci/check-js-generated.sh, fail-fast withset -euo pipefail) regenerates and fails the build if any committedjs/output is stale.tsconfig.json: target es6 → es2020, drop the deprecatedsuppressImplicitAnyIndexErrors.module.tsand the committed declarations (removed deadIDisplay/startup/shutdown, addedcopyFilters/getMediaState/getItemsInRange/active, removed a duplicatesetFilterOrder, etc.).Typing accuracy (from PR review, verified against
obs-studio-client/source):IProperty.next()/previous()andIProperties.first()/last()/get()return undefined at end-of-list / empty / not-found → typedIProperty | undefined; fixed stale JSDoc.sendMessage()moved fromISourcetoIInput(only Input registers it natively).load()hoisted to theISourcebase (registered on Input/Filter/Scene/Transition); per-interface duplicates removed.IModuleFactory.modules()→string[];IModule.initialize()→boolean.;after enum/interface blocks so generatedmodule.jshas no empty statements.Motivation and Context
The committed
js/outputs were hand-maintained and had drifted frommodule.ts. This PR removes the manual step (generate + CI-enforce) and corrects the drifted/inaccurate type declarations.How Has This Been Tested?
yarn build:javascript— clean compile (tsc exit 0).node --check js/module.jsandnode --check js/type_check.js— OK.ci/check-js-generated.shreports a clean tree after regeneration.Types of changes
Checklist: