test+refactor!: API contract test scaffold + fixes for #1202#1204
test+refactor!: API contract test scaffold + fixes for #1202#1204msluszniak wants to merge 11 commits into
Conversation
Adds Jest-based scaffold and two representative tests that catch drift across the package's public API surface. The tests intentionally do not exercise the JSI runtime — they discover modules/hooks via the index exports and assert shared structural contracts. Refs #1018
Two more layers on the API contract scaffold:
- hookContracts.test.ts: compile-time assertion that every public
useXxx hook returns at least { error, isReady, isGenerating,
downloadProgress }. Drift in any hook surfaces as a tsc error
naming the offending hook.
- modelUrls.test.ts: walks every accessor in the model registry,
collects every string field that looks like a URL, and asserts
each one is a non-empty https URL pointing at the
software-mansion HuggingFace org.
Refs #1018, #1202.
Three more API consistency layers: - errorCodes.test.ts: walks RnExecutorchErrorCode and asserts every entry is a unique non-negative integer with a working reverse lookup, and that constructing RnExecutorchError(code) produces a non-empty message. - ttsVoices.test.ts: walks every Kokoro voice constant and asserts the voice variable-name region (e.g. KOKORO_FRENCH_*) matches the phonemizerConfig.lang, that the voiceSource URL points at the voices/ directory, and that every phonemizer URL lives under the matching /phonemizer/<lang>/ tree. Catches copy-paste bugs across voice configs. - apiSurface.test.ts: snapshots the sorted list of public exports from src/index.ts. Accidental adds/removals show up in the diff; intentional changes need --updateSnapshot. Refs #1018, #1202.
…ests Three more API consistency layers: - hookPropsContract.test.ts: compile-time check that every *Props type exposes preventLoad?: boolean, and that every public useXxx hook takes a single object argument. Surfaces useTextToSpeech as the lone two-arg outlier. - registryHookCompatibility.test.ts: compile-time assertion that every category sample from the model registry is assignable to the matching hook's model prop type. Catches drift between the registry's static return shape and the hook prop shapes. - modulePrototype.test.ts: walks each concrete module's prototype chain (using property descriptors so accessor getters aren't invoked) and asserts at least one public method is reachable and delete() is callable. Also snapshots BaseModule's intrinsic surface so silent additions/renames there fail loudly. Surfaces TokenizerModule's missing delete() as a documented opt-out in SKIPS_DELETE (tracked in #1202). Refs #1018, #1202.
- moduleConstruction.test.ts: mocks the ResourceFetcher adapter and constructs every from*-factory-bearing module against a sample config from the registry. Asserts the awaited result is the expected instance and that delete() is callable on the stubbed native module. - moduleHookSignatureAlignment.test.ts: compile-time alignment check between non-generic module prototype methods and the matching hook return field. Catches drift between e.g. LLMModule.prototype.generate and useLLM().generate. Surfaces the TextToImageModule.forward → useTextToImage().generate rename via a dedicated row. - setup-globals.ts: the stubbed loadXxx now resolves to a native module shape with unload() and generateFromFrame() so module delete() and VisionModule's worklet getter work in tests. - .github/workflows/ci.yml: adds a `test` job that runs typecheck:tests and `jest --ci` so the contract suite gates PRs. Refs #1018, #1202.
Source-level fixes for every actionable finding in #1202. The contract suite from #1203 now enforces these without exception sets. Breaking changes: - Renamed useExecutorchModule → useExecutorch to match the use<ModuleStem> convention used by every other hook. - useTextToSpeech now takes a single object argument ({ model, preventLoad }) like every other hook, instead of (model, { preventLoad }). - Renamed useTextToImage().generate → .forward so the hook return field matches the module method name. - ExecutorchModule + TokenizerModule now construct via static factories (fromModelSource, fromModelName) instead of new + instance load(). Removed useModule helper. Non-breaking fixes: - OCRModule, VerticalOCRModule, LLMModule, SpeechToTextModule, TextToSpeechModule, TokenizerModule now extend BaseModule. TokenizerModule consequently gains a delete() that releases the native tokenizer handle (was leaking). Demo apps (apps/speech/*, apps/computer-vision/text_to_image) and the contract tests updated for the new shapes. apiSurface snapshot regenerated. Closes #1202.
msluszniak
left a comment
There was a problem hiding this comment.
Also why CI workflow wasn't triggered by this PR?
| llm_generate: { | ||
| inputs: true as EqualParam<LLMModule['generate'], LLMType['generate']>, | ||
| returns: true as EqualReturn<LLMModule['generate'], LLMType['generate']>, | ||
| }, | ||
| styleTransfer_forward: { | ||
| inputs: true as EqualParam< | ||
| StyleTransferModule['forward'], | ||
| StyleTransferType['forward'] | ||
| >, | ||
| returns: true as EqualReturn< | ||
| StyleTransferModule['forward'], | ||
| StyleTransferType['forward'] |
There was a problem hiding this comment.
llm has generate but the rest has forward, why? We need to think if we want to keep it this way.
EDIT:
Ok, the problem with LLM is that LLMModule exposes both generate and forward and generate from useLLM utilizes LLMModule's generate. I'm ok with both current and changed version, just want to make sure everybody is ok with the chosen version.
- Consolidate split `import type { … } from '../../src'` blocks in
hookContracts.test.ts and moduleHookSignatureAlignment.test.ts.
- Document why LLMModule's primary method is `generate` (not
`forward`) in the signature-alignment table — streaming HF/llama.cpp
convention vs PyTorch single-pass forward.
|
Ok, tested and all functionalities that are changed worked: demo apps + custom app that covered |
| * @param props - Configuration object containing `modelSource` and optional `preventLoad` flag. | ||
| * @returns Ready to use Executorch module. | ||
| */ | ||
| export const useExecutorch = ({ |
There was a problem hiding this comment.
This rename is a breaking change.
Maybe we can export an alias for the patch releases.
"export const useExecutorchModule = useExecutorch"
We could get rid of it when 1.0 rolls out
There was a problem hiding this comment.
We can do that, but please mind the fact that this is not the only breaking change in this PR.
There was a problem hiding this comment.
I know, I'm still reading the changes though.
I feel like the hook and generate->forward renames have the most impact for the end user. We should handle it gracefully if possible
There was a problem hiding this comment.
I'm in favour of making aliases for generate and useExecutorchModule and useTextToSpeech with different signature.
There was a problem hiding this comment.
llm has generate but the rest has forward, why? We need to think if we want to keep it this way.
This way we can also rename this function in llm from generate to forward if we want, similarly to textToImage.
There was a problem hiding this comment.
Yeah, two birds with one stone kind of situation. Let's go with the aliases til 1.0
There was a problem hiding this comment.
Ok, the problem with LLM is that LLMModule exposes both generate and forward, and generate from useLLM utilizes LLMModule's generate. I'm okay with both current and changed version, just want to make sure everybody is ok with the chosen version.
…ech as deprecated aliases Restores backward compatibility for the three external API symbols that were renamed in 88de1f4: - useTextToImage().generate is back as a deprecated alias for .forward. - useExecutorchModule is back as a deprecated re-export of useExecutorch. - useTextToSpeech still prefers the single-object-arg form, but the old (model, { preventLoad }) signature is restored via an overload tagged @deprecated. All three are documented as deprecated and will be removed in a future release. Contract tests updated to cover the deprecated alias hooks alongside the canonical ones; apiSurface snapshot includes the restored useExecutorchModule export.
Adds OCR, VerticalOCR, SpeechToText, TextToSpeech, Tokenizer, and ExecutorchModule rows alongside the existing twelve. Closes the gap flagged in PR review — the omission was an oversight, not deliberate. Construction count goes from 12 to 18.
benITo47
left a comment
There was a problem hiding this comment.
I have no remarks other than the already fixed ones.
I'm approving but someone should still take a second look
Description
Bundles two related changes so the API surface and its contract tests land together:
packages/react-native-executorch/__tests__/api/) — 12 Jest suites + a CItestjob. Catches drift across the public TypeScript surface (modules, hooks, types, registry, error codes, voices, URLs) without exercising native code.Introduces a breaking change?
Type of change
Tested on
Testing instructions
yarn workspace react-native-executorch typecheck:tests yarn workspace react-native-executorch test yarn typecheck yarn lintExpected: 12 suites green, 980 tests passed, 0 skipped, 1 snapshot. The two demo apps that exercise the renamed APIs (
apps/speech,apps/computer-vision/text_to_image) build and run with unchanged behaviour.Screenshots
N/A — no UI changes.
Related issues
Closes #1018 (TS API tests). Closes #1202 (findings the tests surfaced). Supersedes #1203.
Checklist
Additional notes
Breaking API changes:
ExecutorchModuleandTokenizerModulenow construct via static factories (fromModelSource,fromModelName); the instanceload()is removed. TheuseModulehook helper is deleted alongside them.Renames kept as deprecated aliases (callers don't have to migrate yet, but
@deprecatedJSDoc flags new usage):useExecutorchis the new canonical name;useExecutorchModuleis re-exported as a deprecated alias.useTextToSpeech({ model, preventLoad })is the canonical single-arg form; the previoususeTextToSpeech(model, { preventLoad })two-arg signature is preserved as a deprecated overload.useTextToImage().forwardis the canonical hook return field (matchesTextToImageModule.forward);.generateis preserved as a deprecated alias.Non-breaking fixes:
OCRModule,VerticalOCRModule,LLMModule,SpeechToTextModule,TextToSpeechModule,TokenizerModulenow extendBaseModule.TokenizerModuleconsequently gains a workingdelete()(was leaking the native tokenizer handle).Docs + skill files updated for the renames (hand-written only;
docs/docs/06-api-reference/**regenerates from JSDoc on the next typedoc build).Test suite layout
from*factory,useXxxhook exportuseXxxreturn type ⊇{error, isReady, isGenerating, downloadProgress}(compile-time)https://on the software-mansion HF orgphonemizerConfig.lang↔ phonemizer URL paths*PropshaspreventLoad?: boolean; every hook takes a single object arg (compile-time)modelprop (compile-time)delete()reachable;BaseModulesurface snapshotResourceFetcher, constructs each module via itsfrom*factory, asserts instance +delete()(runtime)