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

[#1936] Migrate store.js to TypeScript #1937

Merged
merged 11 commits into from
Mar 19, 2023

Conversation

vvidday
Copy link
Contributor

@vvidday vvidday commented Mar 11, 2023

Part of #1936

Proposed commit message

Currently, despite the addition of TypeScript support, the frontend is
still largely written in JavaScript. This results in a lack of type
safety and many complex objects being passed around as unknown types,
which may in turn lead to errors.

Let's migrate the store.js file to TypeScript, to get types for the all
the states that are stored within the Vuex store. This will provide a
good starting point to migrate the Vue files that access and mutate the
state of the store.

Other information

The declaration of the module and ComponentCustomProperties in vuex.d.ts provides type definitions to the this.$store object that's used in the .vue files.

Also, although I have checked on my end, this PR should probably not be merged until after #1913 , so we can see the updated results of the CI that includes the new TypeScript rules.

@vvidday vvidday marked this pull request as ready for review March 11, 2023 16:50
@vvidday vvidday requested a review from a team March 11, 2023 16:50
Copy link
Member

@ckcherry23 ckcherry23 left a comment

Choose a reason for hiding this comment

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

Thanks for your work! I just have a few questions about typing.

frontend/src/store/store.ts Outdated Show resolved Hide resolved
frontend/src/store/store.ts Outdated Show resolved Hide resolved
frontend/src/types/vuex.d.ts Outdated Show resolved Hide resolved
Copy link
Member

@ckcherry23 ckcherry23 left a comment

Choose a reason for hiding this comment

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

LGTM!

@ckcherry23 ckcherry23 requested a review from a team March 13, 2023 06:56
frontend/src/types/vuex.d.ts Outdated Show resolved Hide resolved
@vvidday
Copy link
Contributor Author

vvidday commented Mar 13, 2023

Just added a ESLint exception in c010d83 for the empty interface in preparation for #1913 to be merged - empty interfaces are usually not meaningful, but this case is an exception.

Because the empty/"repeated" interface is inside the declare module block, which is used to type the this.$store within Vue components. We can't export to other files from within the declare module block, hence the need to declare StoreState at the top level (for other TS files), and once more in the declare module block for the Vue-component-specific typings.

Copy link
Contributor

@zhoukerrr zhoukerrr left a comment

Choose a reason for hiding this comment

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

LGTM! @ckcherry23 caught the points that I want to bring up. Thanks for the help!

@dcshzj dcshzj merged commit 46f423f into reposense:master Mar 19, 2023
@github-actions
Copy link
Contributor

The following links are for previewing this pull request:

@vvidday vvidday mentioned this pull request Apr 11, 2023
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants