fix(tts): construct SarvamAIClient, not the non-existent default export - #41
Open
shreyaskarnik wants to merge 1 commit into
Open
fix(tts): construct SarvamAIClient, not the non-existent default export#41shreyaskarnik wants to merge 1 commit into
shreyaskarnik wants to merge 1 commit into
Conversation
Closes #40. `sarvam.ts` destructured `{ default: SarvamAI }` from `sarvamai`, but the package has no default export and its `SarvamAI` export is a namespace object, not a constructor. The client class is `SarvamAIClient`: default: undefined SarvamAI: object SarvamAIClient: function So `new SarvamAI(...)` threw `SarvamAI is not a constructor` for every user with the package correctly installed. The engine had never worked. It went unnoticed because the try/catch wraps only the `import()` and reports any failure as "requires the 'sarvamai' package", while the `new` call sits outside it — so the real failure surfaced as a bare TypeError at a line that looks unrelated to the import. A guard now names the missing export instead of letting a future rename repeat the same confusion. `sarvamai` becomes a devDependency so the engine is testable at all: vitest cannot intercept `vi.mock` for a module that does not resolve. #36 declares it an optional peer, which this does not pre-empt. Not verified: the `convert()` payload underneath, which needs a live API key. The tests assert the call shape, not that Sarvam accepts it. Reported-by: @Joilence
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.
Closes #40. Found by @Joilence during review of #36.
The bug
src/tts/engines/sarvam.tsdestructured thedefaultexport ofsarvamai. There isn't one, andSarvamAIis a namespace object rather than a constructor:new SarvamAI(...)threwSarvamAI is not a constructorfor every user who had the package correctly installed. The Sarvam engine has never worked.Why it survived this long
The
try/catchwraps only theimport()and reports any failure as "requires the 'sarvamai' package". Thenewcall sits outside it, so the real failure surfaced as a bareTypeErrorat a line that looks unrelated to the import.That same over-broad catch bit me while writing the test: vitest's mock factory is hoisted above the file's consts, the resulting TDZ
ReferenceErrorwas swallowed, and it reported as "package not installed".vi.hoisted()is the fix, and it's commented in the test so the next person doesn't lose the same twenty minutes.Changes
SarvamAIClient.SarvamAIClient, throw a message naming the package and the expected version — instead of lettingnew undefined()produce another unrelated-looking TypeError on a future rename.sarvamaiadded as a devDependency. Not incidental: vitest cannot interceptvi.mockfor a module that doesn't resolve, so without it the engine is untestable. Verified directly — the mock silently no-ops and the import throwsMODULE_NOT_FOUND.Relationship to #36
#36 declares
sarvamaian optional peer and rewrites this import to go throughimportOptional(). This doesn't pre-empt either — the devDependency is what #36 adds too, and the constructor fix is orthogonal to how the module is loaded. Expect a trivial conflict inpackage.jsonat most.Not verified
Whether the
convert()payload underneath is also wrong. That needs a live Sarvam API key. The tests assert the call shape — that the client is constructed withapiSubscriptionKey, and that text/lang/voice/pace reachconvert()— not that Sarvam accepts it. Worth a real end-to-end check by someone with a key before trusting the engine.Test plan
5 new tests in
tests/tts/sarvam.test.ts, written test-first. The mock mirrors the real module shape exactly,default: undefinedand all, so a regression back to the default import fails here rather than only in production.SarvamAIClientwith the resolved API keytarget_language_code,speakerandpacetoconvert()—paceasserted specifically because Sarvam has native rate control, sospeedmust not additionally be applied byconvertToWavand compoundaudiosarrayFull suite: 672 pass. The 3 failures are pre-existing and environmental (no Playwright browser binary locally); they pass in CI.