Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Implement download_file in widget driver #12931

Merged
merged 5 commits into from
Aug 30, 2024
Merged

Conversation

weeman1337
Copy link
Contributor

Implement the widget API download_file in the widget driver.

Needs matrix-org/matrix-widget-api#99

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

Comment on lines 632 to 638
return "https://example.com/_matrix/media/v3/download/test_file";
}

return null;
});

fetchMockJest.get("https://example.com/_matrix/media/v3/download/test_file", "test contents");
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't this be

Suggested change
return "https://example.com/_matrix/media/v3/download/test_file";
}
return null;
});
fetchMockJest.get("https://example.com/_matrix/media/v3/download/test_file", "test contents");
return "https://example.com/_matrix/media/v3/download/example.com/test_file";
}
return null;
});
fetchMockJest.get("https://example.com/_matrix/media/v3/download/example.com/test_file", "test contents");

it might not be obvious why this calls the deprecated API, but maintainers will know better than me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wouldn't this be

Done

it might not be obvious why this calls the deprecated API, but maintainers will know better than me.

The implementation relies on Media . Not sure if the test should mock it away or not 🤷

Signed-off-by: Michael Weimann <[email protected]>
@florianduros
Copy link
Contributor

Hi! Can you first of all fix the lint and ts errors?
Thanks!

@florianduros florianduros added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label Aug 28, 2024
@weeman1337
Copy link
Contributor Author

Hi! Can you first of all fix the lint and ts errors? Thanks!

The „lint“ errors are actually type check errors caused by this PR relying on matrix-org/matrix-widget-api#99

@HarHarLinks
Copy link
Contributor

here's described how to test everything together: matrix-org/matrix-widget-api#99 (comment)

@toger5
Copy link
Contributor

toger5 commented Aug 28, 2024

The „lint“ errors are actually type check errors caused by this PR relying on matrix-org/matrix-widget-api#99

I think you should be able to update the package.json to use your widget api pr for the review and revert the change once the widget api is merged.
Than the two PR's can be reviewed simultaneously with both being checked by the linter already.

To be more specific:
Change:
https://github.com/matrix-org/matrix-react-sdk/blob/develop/package.json#L118
to
"matrix-widget-api: "github:nordeck/matrix-widget-api#download-file" (I think)

@florianduros
Copy link
Contributor

The change in the package.json made it worse

@langleyd langleyd added the X-Release-Blocker This affects the current release cycle and must be solved for a release to happen label Aug 29, 2024
@HarHarLinks
Copy link
Contributor

The change in the package.json made it worse

Hello @florianduros, could you please advise us what you need us to do to get this reviewed? Like weeman said, the lint errors are due to the wrong dependency version. How should we fix it?

@florianduros
Copy link
Contributor

florianduros commented Aug 29, 2024

I'll review it in the afternoon. Why not waiting for the matrix-org/matrix-widget-api#99 PR to be released to ask for a review? It can't be merged without it.

@HarHarLinks
Copy link
Contributor

HarHarLinks commented Aug 29, 2024

Why not waiting for the matrix-org/matrix-widget-api#99 PR to be released to ask for a review? It can't be merged without it.

Code for both (and also a widget) is already done so we could test it end to end. We're asking for reviews of both PRs that target element repos. It seems to us to make sense to review both PRs at the same time.


Some info maybe not obvious from the title and OP: this is a (partial) implementation of MSC4039.

@dbkr
Copy link
Member

dbkr commented Aug 30, 2024

I've merged the widgets api PR and released it as 1.9.0 so you'll need tov change the dependency here (or allow edits so we can).

Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

Similar to the widget-api PR, I have a few questions about the MSC but this seems like a god and accurate impl of what it currently says, so I'd say go with it.

@dbkr dbkr added this pull request to the merge queue Aug 30, 2024
Merged via the queue into matrix-org:develop with commit 19f8b44 Aug 30, 2024
26 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements X-Release-Blocker This affects the current release cycle and must be solved for a release to happen Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants