Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(tsconfig): enable noUncheckedIndexedAccess #168

Merged
merged 8 commits into from
Nov 18, 2024
Merged

Conversation

nbsp
Copy link
Member

@nbsp nbsp commented Nov 18, 2024

used ! for places where we either check earlier, or we know for absolutely certain that there's gonna be a value.

@nbsp nbsp requested a review from lukasIO November 18, 2024 09:12
Copy link

changeset-bot bot commented Nov 18, 2024

🦋 Changeset detected

Latest commit: 284bdd0

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

agents/src/ipc/job_main.ts Outdated Show resolved Hide resolved
agents/src/llm/function_context.ts Outdated Show resolved Hide resolved
agents/src/tokenize/basic/hyphenator.ts Outdated Show resolved Hide resolved
agents/src/worker.ts Show resolved Hide resolved
@@ -25,7 +25,7 @@ export default defineAgent({
const recvTask = async () => {
for await (const event of sttStream) {
if (event.type === stt.SpeechEventType.FINAL_TRANSCRIPT) {
console.log(event.alternatives[0].text);
console.log(event.alternatives[0]!.text);
Copy link
Contributor

Choose a reason for hiding this comment

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

is it safe to assume that final transcript will have event.alternatives populated under all circumstances with at least one element?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes because we check for it inside the STT class. this is a one-or-more array

Copy link
Contributor

@lukasIO lukasIO Nov 18, 2024

Choose a reason for hiding this comment

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

we could define it as a one-or-more array on the typescript side then with alternatives: [elementType, ...elementType[]]

Copy link
Member Author

Choose a reason for hiding this comment

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

well actually not quite. it's an empty array in case of {START,END}_OF_SPEECH because it's unused and it's easier to make it empty than have some sort of complicated typescript type depending on enum value for this. but for final transcript it is guaranteed. maybe i can make alternatives |undefined and then the ! would be on it instead for better readability

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good!

Copy link
Member Author

Choose a reason for hiding this comment

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

already pushed!

plugins/openai/src/realtime/realtime_model.ts Outdated Show resolved Hide resolved
@@ -122,7 +122,7 @@ export class ProcJobExecutor extends JobExecutor {
this.#proc!.send({ case: 'initializeRequest', value: { loggerOptions } });
await once(this.#proc!, 'message').then(([msg]: IPCMessage[]) => {
clearTimeout(timer);
if (msg.case !== 'initializeResponse') {
if (msg!.case !== 'initializeResponse') {
Copy link
Contributor

@gching gching Nov 18, 2024

Choose a reason for hiding this comment

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

Suggested change
if (msg!.case !== 'initializeResponse') {
if (!msg || msg.case !== 'initializeResponse') {

Not sure if doing ! is temporary to unblock this PR and we are tackling this later, but I do prefer to just do safety checks instead of depending on ! to guarantee not having runtime issues

In this case, I know msg should always be defined, but in the case msg is nullish anyways, we should probably throw an error?

Copy link
Member Author

Choose a reason for hiding this comment

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

i don't think we should recover in any way from this. this would signify a serious breakage that should probably throw ugly javascript errors rather than quietly/silently continuing on

Copy link
Member Author

Choose a reason for hiding this comment

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

there's not much to add here with our own error that JavaScript wouldn't complain about anyway, regardless

Copy link
Contributor

Choose a reason for hiding this comment

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

there's not much to add here with our own error that JavaScript wouldn't complain about anyway, regardless

Oh definitely agree, makes sense that the runtime error would accomplish the same thing 😄 - sounds good this is nice!

agents/src/job.ts Outdated Show resolved Hide resolved
nbsp and others added 2 commits November 18, 2024 12:57
Co-authored-by: Gavin <gavinchingy@gmail.com>
@nbsp nbsp merged commit 6bff3b0 into main Nov 18, 2024
4 checks passed
@nbsp nbsp deleted the nbsp/chore/unchecked branch November 18, 2024 11:41
@github-actions github-actions bot mentioned this pull request Nov 19, 2024
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.

3 participants