Skip to content

TypeCheck Frontend#2394

Draft
samfreund wants to merge 37 commits intoPhotonVision:mainfrom
samfreund:vue-tsc
Draft

TypeCheck Frontend#2394
samfreund wants to merge 37 commits intoPhotonVision:mainfrom
samfreund:vue-tsc

Conversation

@samfreund
Copy link
Member

@samfreund samfreund commented Mar 11, 2026

Description

We recently had an error that would've been caught by type checking in the frontend (see #2393). This PR implements type checking, in the hope that prior errors will be caught.

Meta

Merge checklist:

  • Pull Request title is short, imperative summary of proposed changes
  • The description documents the what and why, including events that led to this PR
  • If this PR changes behavior or adds a feature, user documentation is updated
  • If this PR touches photon-serde, all messages have been regenerated and hashes have not changed unexpectedly
  • If this PR touches configuration, this is backwards compatible with all settings going back to the previous seasons's last release (seasons end after champs ends)
  • If this PR touches pipeline settings or anything related to data exchange, the frontend typing is updated
  • If this PR addresses a bug, a regression test for it is added
  • If this PR adds a dependency, the license has been checked for compatibility and steps taken to follow it

@samfreund samfreund requested a review from a team as a code owner March 11, 2026 05:27
@github-actions github-actions bot added the frontend Having to do with PhotonClient and its related items label Mar 11, 2026
@samfreund samfreund force-pushed the vue-tsc branch 5 times, most recently from aa75092 to f72c9b7 Compare March 11, 2026 05:39
@samfreund samfreund marked this pull request as draft March 11, 2026 05:44
@samfreund
Copy link
Member Author

samfreund commented Mar 11, 2026

Grrrr, type checking didn't type check (error was introduced and uncaught)

Gold856
Gold856 requested changes Mar 11, 2026
View reviewed changes
Copy link
Member

@Gold856 Gold856 left a comment

Choose a reason for hiding this comment

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

Too much use of as. Try using https://vuejs.org/api/sfc-script-setup.html#generics to fix most of the instances where you need as. We need to ensure the types are actually correct, not just promise that it's correct.

@github-actions github-actions bot added the backend Things relating to photon-core and photon-server label Mar 11, 2026
@samfreund samfreund force-pushed the vue-tsc branch 2 times, most recently from c839cfe to 97a298a Compare March 11, 2026 18:39
@github-actions github-actions bot removed the backend Things relating to photon-core and photon-server label Mar 11, 2026
Gold856
Gold856 requested changes Mar 23, 2026
View reviewed changes
@samfreund samfreund force-pushed the vue-tsc branch 2 times, most recently from 05925d7 to 473ea67 Compare March 23, 2026 19:34
@samfreund samfreund requested a review from Gold856 March 23, 2026 19:35
@samfreund samfreund force-pushed the vue-tsc branch 2 times, most recently from d42f4ae to 46f6355 Compare March 23, 2026 19:45
@Gold856 Gold856 mentioned this pull request Mar 23, 2026
Merged
8 tasks
Gold856
Gold856 requested changes Mar 23, 2026
View reviewed changes
photon-client/src/components/common/pv-select.vue
Comment on lines +2 to +14
export interface SelectItem<TValue extends string | number = string | number> {
name: string | number;
value: string | number;
value: TValue;
disabled?: boolean;
}
const value = defineModel<string | number | undefined>({ required: true });
</script>

<script setup lang="ts" generic="T extends string | number = string | number">
import { computed } from "vue";
import TooltippedLabel from "@/components/common/pv-tooltipped-label.vue";

type PrimitiveItem = string | number;
type SelectItems = ReadonlyArray<SelectItem> | ReadonlyArray<PrimitiveItem>;
Copy link
Member

@Gold856 Gold856 Mar 23, 2026

Choose a reason for hiding this comment

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

Not sure if you're finished with this, but this whole thing feels weird. There shouldn't need to be a second script tag.

Gold856 added a commit that referenced this pull request Mar 23, 2026
@Gold856
Add more linting rules (#2406)
7b240a0 
As a precursor to #2394, add a bunch of linting rules to try and catch
more mistakes/potential code errors/unnecessary code. Add a bunch of
rules from https://eslint.vuejs.org/rules/ in the "uncategorized"
section that seem useful to have.
@samfreund
Copy link
Member Author

samfreund commented Mar 24, 2026 •
edited
Loading

So floating promises got fixed, but I did it by just spamming void. I'm not sure that's the ideal fix, @Gold856 thoughts?

In the case of axiosPost() I think it's fine, but there are some other instances where we don't have that infra around the function we're calling.

samfreund
samfreund commented Mar 24, 2026
View reviewed changes
photon-client/src/components/dashboard/ConfigOptions.vue Outdated
v-for="(tabGroupData, tabGroupIndex) in tabGroups"
:key="tabGroupIndex"
:cols="tabGroupIndex === 1 && useCameraSettingsStore().currentPipelineSettings.doMultiTarget ? 7 : ''"
:cols="tabGroupIndex === 1 && (useCameraSettingsStore().currentPipelineSettings as any).doMultiTarget ? 7 : ''"
Copy link
Member Author

@samfreund samfreund Mar 24, 2026

Choose a reason for hiding this comment

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

I don't love having to use any here, but I don't think there's a better alternative that doesn't significantly complicate it.

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

Reviewers

@Gold856 Gold856 Gold856 requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

frontend Having to do with PhotonClient and its related items

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@samfreund @Gold856