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

[BREAKING] Convert MatrixClient to TypeScript #1718

Merged
merged 33 commits into from
Jun 4, 2021
Merged

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Jun 1, 2021

This is considered breaking due to an API contract change (merge of two classes): it is unknown if external consumers will have extended these classes.

Accompanied by matrix-org/matrix-react-sdk#6132


Reviewer: This is a massive diff, and one that doesn't line up with the old source. I would highly suggest doing a kind of review where simple sanity is checked rather than exact function definitions being translated to the right place.

Also, probably take breaks. It took over a week to write this, so will probably take a few hours to review.

This is unlikely to be reviewable commit-by-commit, sorry.


The approach for this started with optimism and quickly dwindled to being as much as a straight-through conversion as possible. This means that after the first little bit of the type conversion things started getting // TODO: Types instead of interfaces when they weren't plainly obvious. Trying to type everything out would likely make this PR literally impossible to review, so I've settled for comments for now.

This also merges the MatrixBaseApis with MatrixClient given they were always the same thing. Nothing else extended MatrixBaseApis. This also saves us from having to use mixins/multiple extends, which is arguably a win. The downside is the MatrixClient class is even longer now.

This includes a brief pass over jsdoc just to ensure the type warnings line up. No realistic effort was made to improve the state of the documentation as a whole.

Note: Groups have had the absolute bare minimum amount of effort, even going so far as to not document types or put TODOs on them. This is because they're being ripped out sooner than is worth documenting.

TODO:

  • Unbreak linter
  • Unbreak any tests
  • Unbreak build
  • 1-2 passes over the documentation (skipped: time)
  • 1-2 passes over the code style
  • 1-2 passes over obvious types (skipped: time)
  • Validate react-sdk happy
  • Validate element-web happy
  • Validate element-desktop happy

Note: while I have verified that Element Web/Desktop doesn't explode, there's almost certainly areas I missed in manual testing. We should watch for bugs.

@turt2live turt2live changed the title [WIP] Convert MatrixClient to TypeScript [WIP] [BREAKING] Convert MatrixClient to TypeScript Jun 1, 2021
turt2live added a commit to matrix-org/matrix-react-sdk that referenced this pull request Jun 2, 2021
@jryans
Copy link
Collaborator

jryans commented Jun 2, 2021

@turt2live Thanks for working on this big change! 😄

One thing I've noticed with the current branch is Git seems unable to follow the .js -> .ts rename of client.js, so all blame history would be lost, which seems quite unfortunate for such a key file... 😭 Is it possible to add an early commit to the stack that does a simple rename without content changes, and then rebase the work on top of that? Usually there's a higher chance Git can follow along if the renaming vs. large content changes occur in separate commits.

@turt2live
Copy link
Member Author

I'll timebox doing that, but have little hope of it working. The conversion was deliberately done without a rename due to complexity of the source material: blame would have been lost regardless. This also merges two files into one, making it even harder for the blame to operate...

@turt2live turt2live changed the title [WIP] [BREAKING] Convert MatrixClient to TypeScript [BREAKING] Convert MatrixClient to TypeScript Jun 2, 2021
@turt2live
Copy link
Member Author

@jryans the commit history is a bit fucky, but the commit is there now - does it look okay/different to you?

Github doesn't count it as a rename due to the massive file diff (+8432 -6057 on client.ts / client.js), but the commit trail seems to count it in my git tree.

@turt2live turt2live marked this pull request as ready for review June 3, 2021 00:56
@turt2live turt2live requested a review from a team June 3, 2021 00:57
@jryans
Copy link
Collaborator

jryans commented Jun 3, 2021

Thanks, looks like the line history is indeed now preserved across the rename at least locally (where it matters most), so that's great, thanks! 😁

@jryans jryans requested review from jryans and removed request for a team June 3, 2021 14:52
@jryans
Copy link
Collaborator

jryans commented Jun 3, 2021

I've grabbed this one from the queue explicitly... Will start on it after the sync meeting.

Copy link
Collaborator

@jryans jryans left a comment

Choose a reason for hiding this comment

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

Looks great! 😄 Obviously I've only done a quick skim, but this is a great foundation, and I think it would be better to merge sooner (avoiding conflicts) and then improve from there.

@turt2live
Copy link
Member Author

brace for impact

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants