Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Export types describing all specced media event formats #4092

Merged
merged 6 commits into from
Mar 8, 2024

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Mar 5, 2024

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

Generally I'm worried that this is rather a lot of new types in the top level of the js-sdk namespace, which is already somewhat unnavigable. Can we not find some better way of organising this stuff?

src/matrix.ts Outdated Show resolved Hide resolved
src/models/media.ts Outdated Show resolved Hide resolved
src/models/media.ts Outdated Show resolved Hide resolved
src/models/media.ts Outdated Show resolved Hide resolved
src/models/media.ts Outdated Show resolved Hide resolved
@t3chguy
Copy link
Member Author

t3chguy commented Mar 6, 2024

Generally I'm worried that this is rather a lot of new types in the top level of the js-sdk namespace, which is already somewhat unnavigable. Can we not find some better way of organising this stuff?

given how limited named imports are and that we desire to only have one main entrypoint in the js-sdk I'm not sure how you suggest solving this

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@richvdh
Copy link
Member

richvdh commented Mar 6, 2024

we desire to only have one main entrypoint in the js-sdk

I don't think we desire that. I think we desire not to have every single file in the js-sdk being a thing that applications should import, but having a limited number of well-defined things that applications can import seems sensible to me. For example, crypto-api is deliberately designed to be a public interface, and the crypto interfaces are available through that.

The difficulty is not solving it per se. The difficulty is coming up with a sensible structure that makes sense now, particularly given the absolute tyrefire of the current interface.

Still, I think this PR is a worrying step taking us even further in the wrong direction. Maybe we can discuss further at tomorrow's meeting.

@t3chguy
Copy link
Member Author

t3chguy commented Mar 6, 2024

We can't feasibly add any more entrypoints to the js-sdk as the linters in the upper layers are already maxed out on allowlist/exceptions from the work to move everything to single entrypoint. Given that work exists, it seems to go the complete opposite way of what you are suggesting here.

@richvdh
Copy link
Member

richvdh commented Mar 6, 2024

We can't feasibly add any more entrypoints to the js-sdk as the linters in the upper layers are already maxed out on allowlist/exceptions from the work to move everything to single entrypoint

This list ? There's a lot of stuff there that needs to go away. If you need to get rid of one to make room for another, matrix-js-sdk/src/common-crypto should definitely never have been there and looks like it can be safely removed.

@t3chguy
Copy link
Member Author

t3chguy commented Mar 6, 2024

If you need to get rid of one to make room for another, matrix-js-sdk/src/common-crypto should definitely never have been there and looks like it can be safely removed.

In that case can you please fix the places in which you imported it so the lint exception can be removed?

image image

@richvdh
Copy link
Member

richvdh commented Mar 6, 2024

If you need to get rid of one to make room for another, matrix-js-sdk/src/common-crypto should definitely never have been there and looks like it can be safely removed.

In that case can you please fix the places in which you imported it so the lint exception can be removed?

Looks like I forgot to grep the test code. matrix-org/matrix-react-sdk#12321.

@richvdh richvdh removed their request for review March 6, 2024 22:59
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

This stuff doesn't appear to be making it into the documentation, which is problematic. I think maybe we need to add types.ts as an entrypoint in typedoc.json ?

Could we also add a few words to the README about the new entrypoint, and how you're meant to use it from a project that isn't the react-sdk with its weird build process?

src/@types/media.ts Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
@t3chguy
Copy link
Member Author

t3chguy commented Mar 8, 2024

Could we also add a few words to the README about the new entrypoint, and how you're meant to use it from a project that isn't the react-sdk with its weird build process?

Where is the similar bit re crypto-api so I can remain consistent?

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@richvdh
Copy link
Member

richvdh commented Mar 8, 2024

Where is the similar bit re crypto-api so I can remain consistent?

Nobody has had time to write it yet. It should definitely be done, and should probably have been done when we first added crypto-api, but we were still experimenting with this stuff at the time. Its absence is not a good reason to propagate the error.

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy t3chguy requested a review from richvdh March 8, 2024 13:34
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

You've got a conflict, but lgtm otherwise

…chguy/media-event-type

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy t3chguy added this pull request to the merge queue Mar 8, 2024
Merged via the queue into develop with commit 3711ad7 Mar 8, 2024
23 checks passed
@t3chguy t3chguy deleted the t3chguy/media-event-type branch March 8, 2024 21:29
thoraj added a commit to verji/matrix-js-sdk that referenced this pull request Mar 14, 2024
* v31.5.0-rc.0

* Don't re-fetch thread root if we already have it (matrix-org#4088)

The root event of a thread used to arrive with the pagination request, but this was unspecced and so got changed to simply fetch the root event. In many (almost all) cases this shouldn't be necessary because the thread should already have its root event: re-use it if it's already there. This is only in pagination, so there's no reason to believe that the root event would have changed and needs to be re-fetched.

This removes a number of duplicate calls to the /event/ endpoint from the tests.

* Fix race condition with sliding sync extensions (matrix-org#4089)

* Fix race condition with sliding sync extensions

* Fix types on sliding-sync spec test

* Prettier fixes

* Add `.m.rule.is_room_mention` push rule to DEFAULT_OVERRIDE_RULES (matrix-org#4100)

* Add intentional mentions push rules to DEFAULT_OVERRIDE_RULES

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Iterate

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Iterate

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Iterate

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Iterate

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

---------

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Export types describing all specced media event formats (matrix-org#4092)

* Export types describing all specced media event formats

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Iterate PR

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Move types to a dedicated export

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Iterate

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Add readme entry

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

---------

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Update dependency typedoc to v0.25.11 (matrix-org#4102)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* v31.5.0

* Resetting package fields for development

* Temporarily disable broken step in the release process

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* fix automatic DM avatar with functional members (matrix-org#4017)

* fix automatic DM avatar with functional members

* update comments

* lint

* add tests for functional members

* keep functional members out of the public API

- remove public API for functional members, reverting most of 0ce2d82, f9b41f6, e65fb24
- remove tests for functional members public API c114bf5
- add shared functional members getter for both room name and avatar fallback generation

* filter functional members from more candidates

- remove from hero(es)
- remove from previous members

* add tests for fallback avatars with functional members

* Add docstring for getFunctionalMembers

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

* inline getInvitedAndJoinedFunctionalMemberCount

* update comments for getAvatarFallbackMember

* use correct list of heroes in getAvatarFallbackMember

* remove redundant type annotation

* optimize performance of invitedAndJoinedFunctionalMemberCount

* calculate nonFunctionalMemberCount in one step

instead of iterating redundantly

* clean up functional member tests with review feedback

* lint

* Update src/models/room.ts

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

* apply feedback about comments

* non-functional per review, lint

---------

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

---------

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Co-authored-by: RiotRobot <releases@riot.im>
Co-authored-by: David Baker <dbkr@users.noreply.github.com>
Co-authored-by: Daniel Salinas <zzorba@users.noreply.github.com>
Co-authored-by: Michael Telatynski <7t3chguy@gmail.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Kim Brose <2803622+HarHarLinks@users.noreply.github.com>
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
thoraj added a commit to verji/matrix-js-sdk that referenced this pull request Mar 15, 2024
* v31.5.0-rc.0

* Don't re-fetch thread root if we already have it (matrix-org#4088)

The root event of a thread used to arrive with the pagination request, but this was unspecced and so got changed to simply fetch the root event. In many (almost all) cases this shouldn't be necessary because the thread should already have its root event: re-use it if it's already there. This is only in pagination, so there's no reason to believe that the root event would have changed and needs to be re-fetched.

This removes a number of duplicate calls to the /event/ endpoint from the tests.

* Fix race condition with sliding sync extensions (matrix-org#4089)

* Fix race condition with sliding sync extensions

* Fix types on sliding-sync spec test

* Prettier fixes

* Add `.m.rule.is_room_mention` push rule to DEFAULT_OVERRIDE_RULES (matrix-org#4100)

* Add intentional mentions push rules to DEFAULT_OVERRIDE_RULES

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Iterate

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Iterate

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Iterate

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Iterate

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

---------

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Export types describing all specced media event formats (matrix-org#4092)

* Export types describing all specced media event formats

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Iterate PR

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Move types to a dedicated export

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Iterate

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Add readme entry

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

---------

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Update dependency typedoc to v0.25.11 (matrix-org#4102)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* v31.5.0

* Resetting package fields for development

* Temporarily disable broken step in the release process

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* fix automatic DM avatar with functional members (matrix-org#4017)

* fix automatic DM avatar with functional members

* update comments

* lint

* add tests for functional members

* keep functional members out of the public API

- remove public API for functional members, reverting most of 0ce2d82, f9b41f6, e65fb24
- remove tests for functional members public API c114bf5
- add shared functional members getter for both room name and avatar fallback generation

* filter functional members from more candidates

- remove from hero(es)
- remove from previous members

* add tests for fallback avatars with functional members

* Add docstring for getFunctionalMembers

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

* inline getInvitedAndJoinedFunctionalMemberCount

* update comments for getAvatarFallbackMember

* use correct list of heroes in getAvatarFallbackMember

* remove redundant type annotation

* optimize performance of invitedAndJoinedFunctionalMemberCount

* calculate nonFunctionalMemberCount in one step

instead of iterating redundantly

* clean up functional member tests with review feedback

* lint

* Update src/models/room.ts

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

* apply feedback about comments

* non-functional per review, lint

---------

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

---------

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Co-authored-by: RiotRobot <releases@riot.im>
Co-authored-by: David Baker <dbkr@users.noreply.github.com>
Co-authored-by: Daniel Salinas <zzorba@users.noreply.github.com>
Co-authored-by: Michael Telatynski <7t3chguy@gmail.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Kim Brose <2803622+HarHarLinks@users.noreply.github.com>
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
thoraj added a commit to verji/matrix-js-sdk that referenced this pull request Mar 15, 2024
* v31.5.0-rc.0

* Don't re-fetch thread root if we already have it (matrix-org#4088)

The root event of a thread used to arrive with the pagination request, but this was unspecced and so got changed to simply fetch the root event. In many (almost all) cases this shouldn't be necessary because the thread should already have its root event: re-use it if it's already there. This is only in pagination, so there's no reason to believe that the root event would have changed and needs to be re-fetched.

This removes a number of duplicate calls to the /event/ endpoint from the tests.

* Fix race condition with sliding sync extensions (matrix-org#4089)

* Fix race condition with sliding sync extensions

* Fix types on sliding-sync spec test

* Prettier fixes

* Add `.m.rule.is_room_mention` push rule to DEFAULT_OVERRIDE_RULES (matrix-org#4100)

* Add intentional mentions push rules to DEFAULT_OVERRIDE_RULES

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Iterate

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Iterate

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Iterate

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Iterate

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

---------

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Export types describing all specced media event formats (matrix-org#4092)

* Export types describing all specced media event formats

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Iterate PR

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Move types to a dedicated export

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Iterate

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Add readme entry

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

---------

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Update dependency typedoc to v0.25.11 (matrix-org#4102)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* v31.5.0

* Resetting package fields for development

* Temporarily disable broken step in the release process

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* fix automatic DM avatar with functional members (matrix-org#4017)

* fix automatic DM avatar with functional members

* update comments

* lint

* add tests for functional members

* keep functional members out of the public API

- remove public API for functional members, reverting most of 0ce2d82, f9b41f6, e65fb24
- remove tests for functional members public API c114bf5
- add shared functional members getter for both room name and avatar fallback generation

* filter functional members from more candidates

- remove from hero(es)
- remove from previous members

* add tests for fallback avatars with functional members

* Add docstring for getFunctionalMembers

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

* inline getInvitedAndJoinedFunctionalMemberCount

* update comments for getAvatarFallbackMember

* use correct list of heroes in getAvatarFallbackMember

* remove redundant type annotation

* optimize performance of invitedAndJoinedFunctionalMemberCount

* calculate nonFunctionalMemberCount in one step

instead of iterating redundantly

* clean up functional member tests with review feedback

* lint

* Update src/models/room.ts

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

* apply feedback about comments

* non-functional per review, lint

---------

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

---------

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Co-authored-by: RiotRobot <releases@riot.im>
Co-authored-by: David Baker <dbkr@users.noreply.github.com>
Co-authored-by: Daniel Salinas <zzorba@users.noreply.github.com>
Co-authored-by: Michael Telatynski <7t3chguy@gmail.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Kim Brose <2803622+HarHarLinks@users.noreply.github.com>
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IMediaEventContent should live in matrix-js-sdk
2 participants