Skip to content

Conversation

@IgorSwat
Copy link
Contributor

@IgorSwat IgorSwat commented Jan 9, 2026

Description

Introduces a breaking change?

  • Yes
  • No

Type of change

  • Bug fix (change which fixes an issue)
  • New feature (change which adds functionality)
  • Documentation update (improves or adds clarity to existing documentation)
  • Other (chores, tests, code style improvements etc.)

Tested on

  • iOS
  • Android

Testing instructions

Screenshots

Related issues

Checklist

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the documentation accordingly
  • My changes generate no new warnings

Additional notes

There are still a few side things to be done on this feature.

Removed unnecessary Log.h include from RnExecutorchInstaller.h
Removed unnecessary iostream include from BaseModel.cpp
Copy link
Member

@msluszniak msluszniak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please apply changes to all applicable places, I didn't repeated these comments. But overall, it looks really cool, these suggestions are just very small nits ;)

@IgorSwat IgorSwat force-pushed the @is/text-to-speech branch from 571fada to 9eb0629 Compare January 9, 2026 13:58
@msluszniak
Copy link
Member

Since you added demo app in this PR, you should also update README.md in the following section:

image

Copy link
Contributor

@chmjkb chmjkb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some nits for the moment, I feel like we need to add docs to have a solid understanding of the public API layer. However, this looks like really a lot of solid work, congrats!

Haven't finished reviewing tho :D

const [readyToGenerate, setReadyToGenerate] = useState(false);

const audioContextRef = useRef<AudioContext | null>(null);
const sourceRef = useRef<any>(null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we type this?

iosOptions: ['defaultToSpeaker'],
});

// Initialize context once
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Initialize context once


const onEnd = async () => {
setIsPlaying(false);
setReadyToGenerate(true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesnt the useEffect above handle this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this deleted?

Comment on lines +5 to +8
export enum TextToSpeechLanguage {
EN_US = 0,
EN_GB = 1,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

S2T shared this as string literal union and I recommend using it instead, so we have a unified approach:

export type SpeechToTextLanguage =
  | 'af'
  | 'sq'
  | 'ar'
// this continues

Comment on lines +10 to +12
// Voice configuration
// So far in Kokoro, each voice is directly associated with a language.
// The 'data' field corresponds to (usually) binary file with voice tensor.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use JSDoc for this kind of comments?
For example:

**
 * Voice configuration
 * * So far in Kokoro, each voice is directly associated with a language.
 * The 'data' field corresponds to (usually) binary file with voice tensor.
 */

Overall I think that this is a good approach for everything we share to the public API as it is easy for the user to figure out what the thing is doing without the docs.

Comment on lines +3 to +6
export const URL_PREFIX =
'https://huggingface.co/software-mansion/react-native-executorch';
const VERSION_TAG = 'resolve/v0.6.0';
// const NEXT_VERSION_TAG = 'resolve/v0.7.0';
export const VERSION_TAG = 'resolve/v0.6.0';
export const NEXT_VERSION_TAG = 'resolve/v0.7.0';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be exported? This makes it possible for the users to import this, which is unnecessary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants