Invalid handlers are still listed in OC.Viewer.availableHandlers

[azul]

Describe the bug
OCA.Viewer.registerHandler({}) will add a handler without id or mimes to OCA.Viewer.availableHandlers.

To Reproduce
Steps to reproduce the behavior:

1. Go to some page that loads the viewer js.
2. Go to the console

OCA.Viewer.availableHandlers.length
// 3
OCA.Viewer.registerHandler({})
// err: Please do NOT wait for the DOMContentLoaded before registering your viewer handler
// err: The following handler doesn't have a valid id
OCA.Viewer.availableHandlers.length
// 4

Expected behavior
Invalid handlers should not be added to the available handlers.

Desktop (please complete the following information):

• OS: Linux
• Browser Chromium
• Version 20

Additional context
We're using the availableHandlers in the Collectives app to open markdown files in a viewer component. This is hacky and probably not the intended way of using this API. However adding an invalid handler to the list of availableHandlers still seems like an error.

[skjnldsv]

Oh right, it should not!

[skjnldsv]

Weird, because we actually return

viewer/src/views/Viewer.vue

Lines 525 to 529 in 267797a

	// checking valid handler id
	if (!handler.id || handler.id.trim() === '' || typeof handler.id !== 'string') {
	console.error('The following handler doesn\'t have a valid id', handler)
	return
	}

EDIT:
Aaah, we should indeed make sure the check is in the Viewer service instead.

viewer/src/services/Viewer.js

Lines 68 to 71 in de081e5

	registerHandler(handler) {
	this._state.handlers.push(handler)
	this._mimetypes.push.apply(this._mimetypes, handler.mimes)
	}

[skjnldsv]

We're using the availableHandlers in the Collectives app to open markdown files in a viewer component. This is hacky and probably not the intended way of using this API. However adding an invalid handler to the list of availableHandlers still seems like an error.

Please link me some code?

[azul]

Thanks for the quick response @skjnldsv 😃

We fixed the issue on our end by coming up with a more robust way of selecting the text app handler:

handler() {
	return OCA.Viewer.availableHandlers.find(h => h.id === 'text')
},

Before we were checking h.mimes.contains('text/markdown') - which caused an error when h.mimes was not set.

This issue is mainly meant to track the unexpected behavior we were seeing.

[skjnldsv]

Please don't do that.
this is not why this API has been designed, if you want a proper implementation of the handlers, please focus on implementing #644 with a proper API to render viewer without the Modal.

I cannot guarantee the stability of your code nor can inform you when it will break on your app.
I'm really advocating against hacks like those. availableHandlers should really not have been made available publicly.
I will most likely remove it in the future when I'll move the registerHandler method inside the Viewer service, meaning we would not need the availableHandlers to be publicly exposed.

[azul]

@skjnldsv What would that proper API look like?
The first thing that comes to my mind would be

OCA.Viewer.open(path, {modal: false})

But without the modal one would need to specify where to render the viewer. So maybe something like

OCA.Viewer.open(path, {el: containerElement})

But i still don't understand how that would work. Right now the ViewerComponent is rendered in the render function of the Vue instance that is attached to the #viewer element.
For the second API to work we'd need a separate Vue instance that would be attached to a different element and only render one handlers component, right?

I suspect this Vue instance would have to be created inside the open function and then attached to the specified element.
I guess it would make sense to do this in a separate API function such as display.

OCA.Viewer.display(path, {el: containerElement})

Does that sound like what you had in mind?

[skjnldsv]

OCA.Viewer.open(path, {el: containerElement})

This looks rather nice :)
Btw, the direct call of open(path) is deprecated and removed for 22 (let me do a pr now) Use the restructuring object calling. OCA.Viewer.open({ path, el: containerElement })`

[skjnldsv]

But i still don't understand how that would work. Right now the ViewerComponent is rendered in the render function of the Vue instance that is attached to the #viewer element.

That would require a slightly different logic.
But if we specify this, it should disable other features like pagination, loadMore, etc etc.
So it's easy to have a dedicated script that mounts the component directly in Viewer.vue

Just have a proper Vue.extend of the currentFile.modal for example 🤷
We do that in other locations already like: https://github.com/nextcloud/nextcloud-vue/blob/a553c341029aedf9fef2437e471e471a5da57e75/src/components/Modal/Modal.vue#L384-L390

[SKB-CGN]

Hi,
i still have the error with NC 22.2.3

[skjnldsv]

@StefCGN should probably be a warning for the first two.
Can you expand to know which app is loading those?

[SKB-CGN]

@skjnldsv Please have a look here.

[skjnldsv]

For the DOMContentLoaded one: #1079

[SKB-CGN]

Means? You are working on?

[azul]

@max-nextcloud Could you up on the new viewer API proposal to render the viewer without the modal in a given element? That would clean up the handling significantly. No more need to inspect the availableHandlers.

[max-nextcloud]

Long time no see ;)

In the meantime:

• collectives started using an API exposed by the text app rather than relying on viewer.
• 69668aa removed the DOMContentLoaded warning in the end.
• Most of the other script loading has been updated with the vue rework.

But the initial report in this issue still holds true:

So I'll prepare a PR that fixes it as proposed in #912 (comment)