feature/feat-migrate-to-typescript #1544

dpschen · 2022-02-15T12:20:52Z

dpschen commented

2022-02-15 12:20:52 +00:00

This pull request:

renames js files to ts
adds lang="ts" to all <script> blocks in .vue files.
wraps the default export of non-<script setup> components with defineComponent().
- for some components it was so easy to convert them to <script setup> that I did that instead.
- some components didn't even need a <script> part, so I removed it (yes, it's optional).

Motivation:
By using ts files we immediately get better typing, without changing anything else.

This pull request: - renames js files to ts - adds `lang="ts"` to all `<script>` blocks in `.vue` files. - wraps the default export of non-`<script setup>` components with `defineComponent()`. - for some components it was so easy to convert them to `<script setup>` that I did that instead. - some components didn't even need a `<script>` part, so I removed it (yes, it's optional). Motivation: By using ts files we immediately get better typing, without changing anything else.

dpschen added 3 commits 2022-02-15 12:20:53 +00:00

fd41aaa846

feat: rename js files to ts

39aa258534

feat: add lang ts to script block

continuous-integration/drone/pr Build is passing Details

008e990e75

feat: use defineComponent wrapper

dpschen changed title from ~~WIP: feature/feat-migrate-to-typescript~~ to feature/feat-migrate-to-typescript

2022-02-15 12:23:05 +00:00

frederick commented

2022-02-15 12:25:39 +00:00

Hi dpschen!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://1544-feature-feat-migrate-to-typescri--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi dpschen! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://1544-feature-feat-migrate-to-typescri--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.

dpschen force-pushed feature/feat-migrate-to-typescript from 008e990e75 to 6bec9e3299

2022-02-15 12:31:44 +00:00

Compare

dpschen force-pushed feature/feat-migrate-to-typescript from 6bec9e3299 to 5cfcc66346

2022-02-15 12:43:51 +00:00

Compare

dpschen referenced this pull request

2022-02-15 12:47:25 +00:00

feat: convert some helpers to typescript #1537

dpschen referenced this pull request

2022-02-15 12:48:13 +00:00

feat: convert navigation to script setup and ts #1536

dpschen requested review from konrad 2022-02-15 13:27:52 +00:00

konrad was assigned by dpschen

2022-02-15 13:27:58 +00:00

saibotk commented

2022-02-16 09:08:05 +00:00

Should we maybe also consider using type-based declaration for eg. props and emits?

https://vuejs.org/guide/typescript/composition-api.html#typing-component-props

I usually like them alot more :)

Should we maybe also consider using type-based declaration for eg. props and emits? https://vuejs.org/guide/typescript/composition-api.html#typing-component-props I usually like them alot more :)

👍 1

dpschen commented

2022-02-16 09:26:47 +00:00

Should we maybe also consider using type-based declaration for eg. props and emits?

https://vuejs.org/guide/typescript/composition-api.html#typing-component-props

I usually like them alot more :)

Absolutely! =)
This pull-request also shouldn't be understood as a full typescript conversion (the title is a bit misleading). Rather I tried to do as little as possible so that the request can be reviewed and merged fast.

We had too many large pull requests that stayed open for a long time and were hard to merge, so I try to split it into many smaller packages that are managable instead. If you look at other pull-requests you'll see that I usually implement typing for props and emits in converted and new components.

Sometimes there is a component where I know that there is another pull request that's beeing worked on. In this case I try to keep the diff as minimal as possible to prevent complex merge conflicts. In the medium to long term the goal is to convert everything to typescript and use the recommended linting settings (I think they also enforce some of the typing).

EDIT: I want to add that I merged multiple pull request in this one together so in the end this results in a mixture between converting some components to ts and just adding lang="ts" / defineComponent / renaming to ts files.

> Should we maybe also consider using type-based declaration for eg. props and emits? > > https://vuejs.org/guide/typescript/composition-api.html#typing-component-props > > I usually like them alot more :) Absolutely! =) This pull-request also shouldn't be understood as a full typescript conversion (the title is a bit misleading). Rather I tried to do as little as possible so that the request can be reviewed and merged fast. We had too many large pull requests that stayed open for a long time and were hard to merge, so I try to split it into many smaller packages that are managable instead. If you look at other pull-requests you'll see that I usually implement typing for props and emits in converted and new components. Sometimes there is a component where I know that there is another pull request that's beeing worked on. In this case I try to keep the diff as minimal as possible to prevent complex merge conflicts. In the medium to long term the goal is to convert everything to typescript and [use the recommended linting settings](https://kolaente.dev/vikunja/frontend/pulls/930) (I think they also enforce some of the typing). **EDIT**: I want to add that I merged multiple pull request in this one together so in the end this results in a mixture between converting some components to ts and just adding `lang="ts"` / `defineComponent` / renaming to ts files.

👍 2

dpschen commented

2022-02-18 12:03:36 +00:00

I'm trying to incooporate the tsconfig files from https://github.com/vuejs/create-vue (or the compiled version under https://github.com/vuejs/create-vue-templates )

konrad reviewed 2022-02-18 17:09:25 +00:00

src/components/input/button.vue

						
				@ -18,9 +18,11 @@

				</template>

				<script lang="ts">

konrad commented

2022-02-18 17:01:00 +00:00

Do we need this script tag here at all?

✅ 1

dpschen commented

2022-02-18 17:12:50 +00:00

Just because the name of the button is x-button (which we might want to change). But since this is not what this pull request is about I'd like not to change this here.

Just because the name of the button is `x-button` (which we might want to change). But since this is not what this pull request is about I'd like not to change this here.

👍 1

dpschen marked this conversation as resolved

src/models/linkShare.ts Outdated

						
				@ -0,0 +1,32 @@

				import AbstractModel from './abstractModel'

				import UserModel from './user'

				export default class LinkShareModel extends AbstractModel {

konrad commented

2022-02-18 17:04:59 +00:00

nice catch!

dpschen commented

2022-02-18 17:13:37 +00:00

I found it because there were weird bugs happening after I imported the wrond model :D

dpschen marked this conversation as resolved

src/registerServiceWorker.ts Outdated

						
				@ -0,0 +3,4 @@

				import {register} from 'register-service-worker'

				if (import.meta.env.PROD) {

					register('/sw.ts', {

konrad commented

2022-02-18 17:06:19 +00:00

Did you test this? AFAIK this won't get compiled so the browser will try to load /sw.ts directly (and fail).

Did you test this? AFAIK this won't get compiled so the browser will try to load `/sw.ts` directly (and fail).

dpschen commented

2022-02-18 17:14:25 +00:00

Will check.

konrad commented

2022-04-02 15:24:12 +00:00

Looks like the sw.js file itself is transpiled but the path here not adjusted automatically. I've adjusted the path in fb2eb4c439, seems to work great.

Looks like the `sw.js` file itself is transpiled but the path here not adjusted automatically. I've adjusted the path in fb2eb4c439580a34533a0bf0ee0adfb8f2d3b02d, seems to work great.

👍 1

dpschen commented

2022-04-09 21:01:44 +00:00

I have found some other issues with the pwa setup.
Will push some WIP changes regarding that soon.

I have found some other issues with the pwa setup. Will push some WIP changes regarding that soon.

👍 1

src/services/abstractService.ts Outdated

						
				@ -0,0 +39,4 @@

						})

						// Set the interceptors to process every request

						const self = this

konrad commented

2022-02-18 17:07:18 +00:00

I'm wondering if the self is even required since we're only using arrow functions here?

I'm wondering if the `self` is even required since we're only using arrow functions here?

✅ 1

dpschen commented

2022-02-18 19:24:50 +00:00

Seems not necessary else this line wouldn't work aswell: a2261a6900/src/services/abstractService.ts (L46)

Seems not necessary else this line wouldn't work aswell: https://kolaente.dev/vikunja/frontend/src/commit/a2261a69006fbc5e44b9fc8949f0e817cdd8078e/src/services/abstractService.ts#L46

dpschen commented

2022-02-18 19:29:03 +00:00

Done in 5fad6bc57f

Done in https://kolaente.dev/vikunja/frontend/commit/5fad6bc57f248641767954e09002ce3297603096

👍 1

dpschen marked this conversation as resolved