feature/convert-abstract-service-to-ts #1798
No reviewers
Labels
No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
No Milestone
No project
No Assignees
3 Participants
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: vikunja/frontend#1798
Loading…
Reference in New Issue
No description provided.
Delete Branch "dpschen/frontend:feature/convert-abstract-service-to-ts"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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!
6c64e16469
to7edd7ae8a9
7edd7ae8a9
toc2b8262be3
c2b8262be3
to345cb07605
112f7c832e
to551f3f54e1
551f3f54e1
to8846f681fa
8846f681fa
to4bc5666101
4bc5666101
toc1f5f92fa1
WIP: feature/convert-abstract-service-to-tsto feature/convert-abstract-service-to-tsThis will probably fail right now.
Not sure why exactly – need to debug.
I would love if we could merge this as fast as possible
Looks like it is failing, yes: https://drone.kolaente.de/vikunja/frontend/9719/1/8
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?
@ -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
andany
?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
vsnull
.But probably better to let stackoverflow do it's work: https://stackoverflow.com/a/51439876
:P
@ -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 globalformatDate
helper?Edit: looks like it is.
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.
@ -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?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!
@ -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 ofIList.id
?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.@ -104,2 +107,2 @@
}
}
Object.assign(this.paths, paths)
Where does
paths
come from?From here.
Or what do you mean?
Oh, yes that's what I meant.
@ -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.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.
@ -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
.See #1798 (comment)
@ -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
.See #1798 (comment)
@ -23,1 +26,4 @@
}
export default store
Why not export the store directly?
Will do, I tried some different method to type store modules where this was necessary. Now it seems not needed any more.
Actually it is still needed. Or at least I don't know how else I could type the store before exporting.
@ -147,3 +172,3 @@
},
addTasksToBucket(state, {tasks, bucketId}) {
addTasksToBucket(state, {tasks, bucketId}: {
Shouldn't the
state
parameter be typed as well here?It should already be typed by the vuex Module type
@ -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.
Meaning
IUser['id']
?Yes.
When trying to open the frontend I get a blank page and this error in the console:
Looking at a log of the list model the property really is undefined:
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:And it looks like there's been problems setting the user settings, not sure if that's related:
50945aad1e
to05729019d5
05729019d5
tofc72965f75
402c18b282
tocc202d0aed
cc202d0aed
toa20c4e6840
266dd64d2e
to1bd157bffc
17ff291ab3
to7b701de2bc
7b701de2bc
to54de368642
Thanks a lot!