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

Merged
konrad merged 32 commits from dpschen/frontend:feature/convert-abstract-service-to-ts into main 4 weeks ago
Collaborator
There is no content yet.
dpschen added 2 commits 6 months ago
Collaborator

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 4 months ago
dpschen force-pushed feature/convert-abstract-service-to-ts from 7edd7ae8a9 to c2b8262be3 3 months ago
dpschen force-pushed feature/convert-abstract-service-to-ts from c2b8262be3 to 345cb07605 3 months ago
dpschen added 11 commits 3 months ago
dpschen force-pushed feature/convert-abstract-service-to-ts from 112f7c832e to 551f3f54e1 3 months ago
dpschen force-pushed feature/convert-abstract-service-to-ts from 551f3f54e1 to 8846f681fa 3 months ago
dpschen force-pushed feature/convert-abstract-service-to-ts from 8846f681fa to 4bc5666101 2 months ago
dpschen force-pushed feature/convert-abstract-service-to-ts from 4bc5666101 to c1f5f92fa1 2 months ago
dpschen requested review from konrad 2 months ago
dpschen changed title from WIP: feature/convert-abstract-service-to-ts to feature/convert-abstract-service-to-ts 2 months ago
dpschen added the
kind/feature
help wanted
labels 2 months ago
Poster
Collaborator

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 2 months ago
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.
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
Owner

Can you rename it?

Can you rename it?
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'
Owner

What's the difference between unknown and any?

What's the difference between `unknown` and `any`?
Poster
Collaborator

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
class="dueDate"
@click.prevent.stop="showDefer = !showDefer"
v-tooltip="formatDate(task.dueDate)"
v-tooltip="formatDateLong(task.dueDate)"
Owner

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.
Poster
Collaborator

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?
Owner

Yeah, removing the mixins is fine.

Yeah, removing the mixins is fine.
konrad marked this conversation as resolved
declare taskId: number
createdBy: IUser
file: IFile
created: Date
Owner

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

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

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.
Owner

Makes sense!

Makes sense!
konrad marked this conversation as resolved
created: Date
updated: Date
listId: IList['id'] // Meta, only used when creating a new task
Owner

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`?
Poster
Collaborator

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
}
}
Object.assign(this.paths, paths)
Owner

Where does paths come from?

Where does `paths` come from?
Poster
Collaborator

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?
Owner

Oh, yes that's what I meant.

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

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.
Poster
Collaborator

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.
Owner
You could create [an anonymous instantiation of the class](https://stackoverflow.com/a/58773062/10924593).
* @returns {Q.Promise<unknown>}
*/
async getM(url, model = {}, params = {}) {
async getM(url : string, model : Model = new AbstractModel({}), params: Record<string, unknown> = {}) {
Owner

Same here for AbstractModel.

Same here for `AbstractModel`.
Poster
Collaborator

See #1798

See https://kolaente.dev/vikunja/frontend/pulls/1798#issuecomment-33807
* @returns {Q.Promise<any>}
*/
async getAll(model = {}, params = {}, page = 1) {
async getAll(model : Model = new AbstractModel({}), params = {}, page = 1) {
Owner

Same here for AbstractModel.

Same here for `AbstractModel`.
Poster
Collaborator

See #1798

See https://kolaente.dev/vikunja/frontend/pulls/1798#issuecomment-33807
}
export default store
Owner

Why not export the store directly?

Why not export the store directly?
Poster
Collaborator

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.
Poster
Collaborator

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
},
addTasksToBucket(state, {tasks, bucketId}) {
addTasksToBucket(state, {tasks, bucketId}: {
Owner

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

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

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
} as const
export interface Info {
id: number // what kind of id is this?
Owner

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

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

Meaning IUser['id']?

Meaning `IUser['id']`?
Owner

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 2 months ago
dpschen force-pushed feature/convert-abstract-service-to-ts from 05729019d5 to fc72965f75 2 months ago
dpschen force-pushed feature/convert-abstract-service-to-ts from 402c18b282 to cc202d0aed 2 months ago
dpschen force-pushed feature/convert-abstract-service-to-ts from cc202d0aed to a20c4e6840 2 months ago
konrad added 2 commits 4 weeks ago
konrad added 1 commit 4 weeks ago
266dd64d2e
Merge branch 'main' into feature/convert-abstract-service-to-ts
dpschen force-pushed feature/convert-abstract-service-to-ts from 266dd64d2e to 1bd157bffc 4 weeks ago
dpschen added 1 commit 4 weeks ago
17ff291ab3
fix: use typed useStore
dpschen force-pushed feature/convert-abstract-service-to-ts from 17ff291ab3 to 7b701de2bc 4 weeks ago
dpschen force-pushed feature/convert-abstract-service-to-ts from 7b701de2bc to 54de368642 4 weeks ago
konrad approved these changes 4 weeks ago
konrad left a comment
Owner

Thanks a lot!

Thanks a lot!
konrad merged commit dbea1f7a51 into main 4 weeks ago
konrad deleted branch feature/convert-abstract-service-to-ts 4 weeks ago

Reviewers

konrad approved these changes 4 weeks ago
continuous-integration/drone/pr Build is passing
The pull request has been merged as dbea1f7a51.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/frontend#1798
Loading…
There is no content yet.