Skip to content

chore: migrate composables to typescript#2497

Merged
mejo- merged 3 commits into
mainfrom
chore/composables_typescript
May 12, 2026
Merged

chore: migrate composables to typescript#2497
mejo- merged 3 commits into
mainfrom
chore/composables_typescript

Conversation

@mejo-
Copy link
Copy Markdown
Member

@mejo- mejo- commented May 11, 2026

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits

🤖 AI (if applicable)

  • The content of this PR was partly generated using AI tools
  • The AI-generated content was reviewed, comprehended and tested by me

mejo- added 2 commits May 11, 2026 20:06
Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
@mejo- mejo- self-assigned this May 11, 2026
@github-project-automation github-project-automation Bot moved this to 🧭 Planning evaluation (don't pick) in 📝 Productivity team May 11, 2026
@mejo- mejo- moved this from 🧭 Planning evaluation (don't pick) to 🏗️ In progress in 📝 Productivity team May 11, 2026
@mejo- mejo- force-pushed the chore/composables_typescript branch from 9da1d63 to 34e6c7a Compare May 11, 2026 18:19
@mejo- mejo- moved this from 🏗️ In progress to 👀 In review in 📝 Productivity team May 11, 2026
Comment thread src/composables/useColor.ts Outdated
Comment thread src/composables/useReader.ts
Comment thread src/composables/useReader.ts Outdated
Comment on lines +150 to +156
interface PageInfoBarElement extends Element {
currentPage: PageInfo | null | undefined
canEdit: boolean
attachmentCount: number
backlinkCount: number
}
const el = readerEl.value?.querySelector<PageInfoBarElement>('page-info-bar')
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks somewhat odd to me. It's an HTML Element, right? And then it has these additional properties of currentPage, canEdit, etc. Seems like we are mixing two things up here.

Copy link
Copy Markdown
Member Author

@mejo- mejo- May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PageInfoBar is a custom element and yes, it has these props. At least that's my understanding.

Though it seems like currentPage is not visible in dom structure, only the others are. When you look at the dom structure, it looks like this:

image

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a bit further research on this.

Vue custom elements expose their component props as JavaScript properties on the DOM element. So e.g. using el.attachmentCount = 3 is the correct API usage.

Though only props with simple types (string, number, boolean) get exposed as HTML attributes in the DOM, complex attributes are only available as JavaScript properties. That's why currentPage is not visible in the DOM.

See also https://vuejs.org/guide/extras/web-components.html#passing-dom-properties and https://web.dev/articles/custom-elements-best-practices.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the interface to the top of the file and changed extends Element to extends HTMLElement as that's indeed more accurate.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah... I see. Could we move the defineCustomElement to the top of the composable then and use PageInfoBarCE to get the type?

const PageInfoBarCE = defineCustomElement(PageInfoBar, { shadowRoot: false })
type PageInfoBarType = InstanceType<typeof PageInfoBarCE>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Based on https://vuejs.org/guide/typescript/composition-api.html#typing-component-template-refs for typing of refs which also kind of applies here i think.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already merged, but maybe something we can improve later on? I gave your approach a quick test and it didn't work out of the box:

With type PageInfoBarType = InstanceType<typeof PageInfoBarCE> and const el = readerEl.value?.querySelector<PageInfoBarType>('page-info-bar'), el.attachmentCount = attachmentCount.value results in

TS2339: Property attachmentCount does not exist on type VueElement

Copy link
Copy Markdown
Collaborator

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for looking into this.

I already commented outside the review as I was not sure if I was gonna finish it. Mostly minor things. One type annotation looks off to me though..

Signed-off-by: Jonas <jonas@freesources.org>
@mejo- mejo- force-pushed the chore/composables_typescript branch from 34e6c7a to 3a17cd9 Compare May 12, 2026 07:40
@mejo- mejo- merged commit 4fd5ad2 into main May 12, 2026
61 checks passed
@mejo- mejo- deleted the chore/composables_typescript branch May 12, 2026 08:03
@github-project-automation github-project-automation Bot moved this from 👀 In review to ☑️ Done in 📝 Productivity team May 12, 2026
lipbalmi pushed a commit to lipbalmi/collectives that referenced this pull request May 13, 2026
Follow up for nextcloud#2497 (comment)

Signed-off-by: Max <max@nextcloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: ☑️ Done

Development

Successfully merging this pull request may close these issues.

3 participants