feature/convert-abstract-service-to-ts #1798

Merged
konrad merged 32 commits from dpschen/frontend:feature/convert-abstract-service-to-ts into main 2022-09-06 09:26:49 +00:00
Member
No description provided.
dpschen added 2 commits 2022-04-10 00:18:14 +00:00
Member

Hi dpschen!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://1798-feature-convert-abstract-service--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://1798-feature-convert-abstract-service--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/convert-abstract-service-to-ts from 6c64e16469 to 7edd7ae8a9 2022-05-23 21:55:34 +00:00 Compare
dpschen force-pushed feature/convert-abstract-service-to-ts from 7edd7ae8a9 to c2b8262be3 2022-06-21 22:38:53 +00:00 Compare
dpschen force-pushed feature/convert-abstract-service-to-ts from c2b8262be3 to 345cb07605 2022-07-04 21:35:23 +00:00 Compare
dpschen added 11 commits 2022-07-04 21:59:07 +00:00
dpschen force-pushed feature/convert-abstract-service-to-ts from 112f7c832e to 551f3f54e1 2022-07-07 18:20:55 +00:00 Compare
dpschen force-pushed feature/convert-abstract-service-to-ts from 551f3f54e1 to 8846f681fa 2022-07-20 19:27:49 +00:00 Compare
dpschen force-pushed feature/convert-abstract-service-to-ts from 8846f681fa to 4bc5666101 2022-07-21 17:41:00 +00:00 Compare
dpschen force-pushed feature/convert-abstract-service-to-ts from 4bc5666101 to c1f5f92fa1 2022-08-04 20:40:55 +00:00 Compare
dpschen requested review from konrad 2022-08-04 20:49:33 +00:00
dpschen changed title from WIP: feature/convert-abstract-service-to-ts to feature/convert-abstract-service-to-ts 2022-08-04 20:49:42 +00:00
dpschen added the
kind/feature
help wanted
labels 2022-08-04 20:50:18 +00:00
Author
Member

This will probably fail right now.
Not sure why exactly – need to debug.
I would love if we could merge this as fast as possible

This will probably fail right now. Not sure why exactly – need to debug. I would love if we could merge this as fast as possible
Owner
Looks like it is failing, yes: https://drone.kolaente.de/vikunja/frontend/9719/1/8
konrad reviewed 2022-08-05 06:22:07 +00:00
konrad left a comment
Owner

Mostly comments, nothing too critical. Should be good to go once they are addressed (sometimes an explanation is enough) and CI passes.

Mostly comments, nothing too critical. Should be good to go once they are addressed (sometimes an explanation is enough) and CI passes.
@ -118,3 +118,3 @@
for (const l in labels.value) {
if (labels.value[l].id === label.id) {
labels.value.splice(l, 1)
labels.value.splice(l, 1) // FIXME: l should be index

Can you rename it?

Can you rename it?
@ -48,2 +48,2 @@
const isDate = (e: any) => e instanceof Date
const isString = (e: any) => typeof e === 'string'
const isDate = (e: unknown) => e instanceof Date
const isString = (e: unknown) => typeof e === 'string'

What's the difference between unknown and any?

What's the difference between `unknown` and `any`?
Author
Member

unknown means that we do not know the type. For these function we don't know it.
any means we don't care what type we use.

It reminds me a bit of the difference of undefined vs null.

But probably better to let stackoverflow do it's work: https://stackoverflow.com/a/51439876

:P

`unknown` means that we do not know the type. For these function we don't know it. `any` means we don't care what type we use. It reminds me a bit of the difference of `undefined` vs `null`. But probably better to let stackoverflow do it's work: https://stackoverflow.com/a/51439876 :P
dpschen marked this conversation as resolved
@ -44,3 +44,3 @@
class="dueDate"
@click.prevent.stop="showDefer = !showDefer"
v-tooltip="formatDate(task.dueDate)"
v-tooltip="formatDateLong(task.dueDate)"

Is formatDateLong the same as the global formatDate helper?

Edit: looks like it is.

Is `formatDateLong` the same as the global `formatDate` helper? Edit: looks like it is.
Author
Member

Yes. I removed the mixins and replaced them everywhere with imports for better typing (trying to avoid globals in general). In order to prevent naming collision I had to rename one of the methods. I don't remember anymore where it collided though…

Is the replacing with imports fine with you?

Yes. I removed the mixins and replaced them everywhere with imports for better typing (trying to avoid globals in general). In order to prevent naming collision I had to rename one of the methods. I don't remember anymore where it collided though… Is the replacing with imports fine with you?

Yeah, removing the mixins is fine.

Yeah, removing the mixins is fine.
konrad marked this conversation as resolved
@ -4,0 +15,4 @@
declare taskId: number
createdBy: IUser
file: IFile
created: Date

Why use declare for some properties instead of only defining the property?

Why use `declare` for some properties instead of only defining the property?
Author
Member

I had to use declare everywhere where a property 'isn't defined'. All of them are, but typescript doesn't understand that this happens mostly in the constructor of the abstract service.
Not sure if this here is the right way to do things. Might be one reason why the build fails.

I had to use declare everywhere where a property 'isn't defined'. All of them are, but typescript doesn't understand that this happens mostly in the constructor of the abstract service. Not sure if this here is the right way to do things. Might be one reason why the build fails.

Makes sense!

Makes sense!
konrad marked this conversation as resolved
@ -15,0 +56,4 @@
created: Date
updated: Date
listId: IList['id'] // Meta, only used when creating a new task

Probably my lack of typescript knowledge speaking here: I've seen this a few times now, is there a special reason to use IList['id'] instead of IList.id?

Probably my lack of typescript knowledge speaking here: I've seen this a few times now, is there a special reason to use `IList['id']` instead of `IList.id`?
Author
Member

IList is a type and not a namespace. Afaik you can only access types in namespaces like IList.id. To access properties of types you need that bracket notation.

IList is a type and not a namespace. Afaik you can only access types in namespaces like `IList.id`. To access properties of types you need that bracket notation.
dpschen marked this conversation as resolved
@ -104,2 +107,2 @@
}
}
Object.assign(this.paths, paths)

Where does paths come from?

Where does `paths` come from?
Author
Member

From here.
Or what do you mean?

From [here](https://kolaente.dev/vikunja/frontend/src/commit/c1f5f92fa164637040198c1a1b8de73f9ed9e0c7/src/services/abstractService.ts#L70). Or what do you mean?

Oh, yes that's what I meant.

Oh, yes that's what I meant.
dpschen marked this conversation as resolved
@ -206,2 +197,2 @@
modelFactory(data) {
return data
modelFactory(data : Partial<Model>) {
return new AbstractModel(data)

Since AbstractModel is an abstract class, this won't work. You cannot instantiate abstract classes.

Since `AbstractModel` is an abstract class, this won't work. You cannot instantiate abstract classes.
Author
Member

Makes sense. Any idea how the returned data can have the correct type? Right now the abstract service has no knowledge of the model class that is used. Model here only stands for the extendeded interface, but that can't be instantiated.

Makes sense. Any idea how the returned data can have the correct type? Right now the abstract service has no knowledge of the model class that is used. `Model` here only stands for the extendeded interface, but that can't be instantiated.
You could create [an anonymous instantiation of the class](https://stackoverflow.com/a/58773062/10924593).
@ -310,3 +281,2 @@
* @returns {Q.Promise<unknown>}
*/
async getM(url, model = {}, params = {}) {
async getM(url : string, model : Model = new AbstractModel({}), params: Record<string, unknown> = {}) {

Same here for AbstractModel.

Same here for `AbstractModel`.
Author
Member
See https://kolaente.dev/vikunja/frontend/pulls/1798#issuecomment-33807
@ -344,3 +314,2 @@
* @returns {Q.Promise<any>}
*/
async getAll(model = {}, params = {}, page = 1) {
async getAll(model : Model = new AbstractModel({}), params = {}, page = 1) {

Same here for AbstractModel.

Same here for `AbstractModel`.
Author
Member
See https://kolaente.dev/vikunja/frontend/pulls/1798#issuecomment-33807
@ -23,1 +26,4 @@
}
export default store

Why not export the store directly?

Why not export the store directly?
Author
Member

Will do, I tried some different method to type store modules where this was necessary. Now it seems not needed any more.

Will do, I tried some different method to type store modules where this was necessary. Now it seems not needed any more.
Author
Member

Actually it is still needed. Or at least I don't know how else I could type the store before exporting.

Actually it is still needed. Or at least I don't know how else I could type the store before exporting.
dpschen marked this conversation as resolved
@ -147,3 +172,3 @@
},
addTasksToBucket(state, {tasks, bucketId}) {
addTasksToBucket(state, {tasks, bucketId}: {

Shouldn't the state parameter be typed as well here?

Shouldn't the `state` parameter be typed as well here?
Author
Member

It should already be typed by the vuex Module type

It should already be typed by the [vuex Module type](https://kolaente.dev/vikunja/frontend/src/commit/c1f5f92fa164637040198c1a1b8de73f9ed9e0c7/src/store/modules/kanban.ts#L41)
dpschen marked this conversation as resolved
@ -0,0 +28,4 @@
} as const
export interface Info {
id: number // what kind of id is this?

I think that's the user id of the currently authenticated user.

I *think* that's the user id of the currently authenticated user.
Author
Member

Meaning IUser['id']?

Meaning `IUser['id']`?

Yes.

Yes.
Owner

When trying to open the frontend I get a blank page and this error in the console:

image

When trying to open the frontend I get a blank page and this error in the console: ![image](/attachments/c22a34e2-d608-4513-a3df-58454ee539de)
Owner

Looking at a log of the list model the property really is undefined:

image

Looking at a log of the list model the property really is undefined: ![image](/attachments/dbe06d64-c3c5-4922-8917-47ccf7c86905)
4.2 KiB
Owner

This is probably the problem the tests catch. Once I solved this (quick and dirty with (this.tasks ?? []).map( the frontend loaded but I got a bunch of similar errors:

image

image

And it looks like there's been problems setting the user settings, not sure if that's related:

image

image

This is probably the problem the tests catch. Once I solved this (quick and dirty with `(this.tasks ?? []).map(` the frontend loaded but I got a bunch of similar errors: ![image](/attachments/c367baae-9a80-4d96-bddb-7d44a6e98870) ![image](/attachments/07307e46-a53c-4acf-96a1-426b7863f178) And it looks like there's been problems setting the user settings, not sure if that's related: ![image](/attachments/a5e0e70e-6dc0-4dfe-a7fe-09cc34ece5b1) ![image](/attachments/a5bf4dc6-83e0-4d79-9e7c-865ca8745e43)
dpschen force-pushed feature/convert-abstract-service-to-ts from 50945aad1e to 05729019d5 2022-08-14 13:32:13 +00:00 Compare
dpschen force-pushed feature/convert-abstract-service-to-ts from 05729019d5 to fc72965f75 2022-08-14 13:33:20 +00:00 Compare
dpschen force-pushed feature/convert-abstract-service-to-ts from 402c18b282 to cc202d0aed 2022-08-14 16:07:50 +00:00 Compare
dpschen force-pushed feature/convert-abstract-service-to-ts from cc202d0aed to a20c4e6840 2022-08-14 16:26:21 +00:00 Compare
dpschen force-pushed feature/convert-abstract-service-to-ts from 266dd64d2e to 1bd157bffc 2022-09-05 16:53:35 +00:00 Compare
dpschen force-pushed feature/convert-abstract-service-to-ts from 17ff291ab3 to 7b701de2bc 2022-09-05 17:01:55 +00:00 Compare
dpschen force-pushed feature/convert-abstract-service-to-ts from 7b701de2bc to 54de368642 2022-09-05 18:11:16 +00:00 Compare
konrad approved these changes 2022-09-06 09:25:42 +00:00
konrad left a comment
Owner

Thanks a lot!

Thanks a lot!
konrad merged commit dbea1f7a51 into main 2022-09-06 09:26:49 +00:00
konrad deleted branch feature/convert-abstract-service-to-ts 2022-09-06 09:26:49 +00:00
This repo is archived. You cannot comment on pull requests.
No description provided.