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 app.vue to TypeScript #1938

Merged
merged 21 commits into from Mar 21, 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 root component, app.vue, to TypeScript. As this is the
entry point of the app, converting this file to TypeScript will enable
the conversion of other Vue components.

Other information

The diff might look confusing because of the re-ordering of the options (apologies for missing this in #1867), please look at 4bfc3aa to see the actual diff of the TypeScript conversion.

I had to use any to define the app property of window.app - not too sure if it's possible to type this.

Lastly, regarding the topic of combining/separating PRs, I was not too sure whether to separate this from #1937. Because of the nature of TypeScript migrations, the PRs would probably be mostly sequential (e.g. this PR relies on the previous PR for the this.$store to be typed), and hence also contains the changes of that PR. This might result in more work, such as requiring maintainers to merge a specific PR before another, as well as any changes to the preceding PR might have to be merged back into this one. I guess there's a tradeoff between the above and keeping the PR's to 'one small change'. Currently, I leaned towards separating them as each change is fairly substantial and they are two separate files. Would love to get any feedback/opinions on this.

@vvidday vvidday marked this pull request as ready for review March 11, 2023 17:01
@vvidday vvidday requested a review from a team March 11, 2023 17:01
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.

Great job on getting the TypeScript migration underway! I noticed that there are a few objects that could benefit from having explicit types as they are destructured later on. Left some comments to that effect.

frontend/src/app.vue Show resolved Hide resolved
frontend/src/app.vue Show resolved Hide resolved
frontend/src/app.vue Show resolved Hide resolved
@vvidday vvidday requested a review from ckcherry23 March 17, 2023 13:49
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 18, 2023 07:27
@HCY123902 HCY123902 requested a review from a team March 19, 2023 09:00
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

@dcshzj dcshzj merged commit 32d49ad into reposense:master Mar 21, 2023
10 checks passed
@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