Implement MSC4039 download_file action

[weeman1337]

Adds support for the download_file action according to MSC4039.

It contains neither the timeout nor the encryption part. This can be done in subsequent PRs.

[HarHarLinks]

other than that, works for me

[HarHarLinks]

test setup:

#!/usr/bin/env bash
# using node 20.16.0

git clone git@github.com:nordeck/matrix-widget-api.git
cd matrix-widget-api
git checkout download-file # d64636a9
yarn link 
yarn install
yarn build
cd ..

git clone git@github.com:nordeck/matrix-widget-toolkit.git
cd matrix-widget-toolkit
git checkout nic/feat/NEO-721-downlad-file-api # d3462dd3
yarn link matrix-widget-api
yarn install
# yarn dev:example
cd ..

git clone git@github.com:matrix-org/matrix-js-sdk.git
cd matrix-js-sdk
# git checkout develop # 2a6612c7
yarn link matrix-widget-api
yarn link
yarn install
cd ..

git clone git@github.com:nordeck/matrix-react-sdk.git
cd matrix-react-sdk
git checkout download_file # 5ca0745f
yarn link matrix-widget-api
yarn link matrix-js-sdk
yarn link
yarn install
cd ..

git clone git@github.com:element-hq/element-web.git
cd element-web
# git checkout develop # addb3352
yarn link matrix-widget-api
yarn link matrix-js-sdk
yarn link matrix-react-sdk
yarn install
# yarn start

[dbkr]

Why does this need to be an exception? Is it taken directly from the wire and we don't want to convert?

[HarHarLinks]

same as e.g.

matrix-widget-api/src/interfaces/OpenIDCredentialsAction.ts

Line 24 in 3c9543c

original_request_id: string; // eslint-disable-line camelcase

[dbkr]

OK, careful of cargoculting then - the other instance has zero comments on why the exception exists too. Doesn't feel like a blocker though.

[dbkr]

I guess this is more a comment on the MSC, but maybe body would be a better name for this? file implies https://developer.mozilla.org/en-US/docs/Web/API/File which is not one of the many things this type can be. Probably doesn't need to match the upload type (although that isn't a file object either...)

[dbkr]

Also, another comment that's really on the MSC, but... why XMLHttpRequestBodyInit? This seems like a bit of a weird union type defined in the fetch API for... actually I'm not even sure why, it doesn't seem like it's referenced. The react-sdk impl just sends a blob. If something sends ones of the other types, how would it go about determining what it's actually got? instanceof in js can be... special.

[HarHarLinks]

The explanations I can offer is, quoting from the MSC:

To your first question:

response.file mirrors data.file of the upload_file action and is a XMLHttpRequestBodyInit that is supported as a data type in the postMessage API.

To be consistent, would you think that we would also have to rename the upload's data.file? We would then need to spend some thoughts on backwards compatibility.

To the second one:
XMLHttpRequestBodyInit is reused again here because it was already used in

matrix-widget-api/src/interfaces/UploadFileAction.ts

Line 23 in 3c9543c

file: XMLHttpRequestBodyInit;

.

Again, to be consistent we would need to change it there too and manage version incompatibilities.

[dbkr]

Well, IMO renaming file on the upload API would be clearer, but that's for the MSC discussion anyway. If we think the right way to go is to change upload too, then we'd need a new version of the MSC anyway, so we may as well implement download as the MSC is now and then change both at the same time, at a later date?

[dbkr]

Again, more for the MSC but... what about large files? I guess the answer is that since there's no specific limit on postMessage, we can add a different API later if it's needed?

[HarHarLinks]

I don't see an obvious issue with postMessage at a glance. Mostly, we assume that we can do whatever we already can do for uploading. This update to MSC4039 seeks to address Authenticated Media, which got overlooked so far for the widget API. The real answer is probably to revamp the widget API entirely to enable widgets to use their own scoped access tokens to access the C-S API directly, but we won't get there before Matrix 1.12.

[dbkr]

Well, I don't think trying to download a 1GB file over the postmessage API is going to be a great user experience, but yeah, if this is more of a stop-gap solution until we have scoped access tokens then that's probably okay.

[dbkr]

In summary I think this seems broadly okay, I'll transfer my MSC-specific comments over the MSC. I'm happy to merge this if you are?

[sonarcloud]

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
94.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[HarHarLinks]

Thank you, I think we're ready to merge.

Full context: weeman is on holiday and the rest of the team from Nordeck is taking over, which is why I'm responding to your comments and pushing changes, but cannot resolve conversations etc.