Skip to content

Improving Matrix for Developers #1546

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

Closed
resynth1943 opened this issue Nov 25, 2020 · 7 comments
Closed

Improving Matrix for Developers #1546

resynth1943 opened this issue Nov 25, 2020 · 7 comments

Comments

@resynth1943
Copy link

Improving Matrix for Developers

TL;DR, for the lazy πŸ’€:

  • convert our library to TypeScript! πŸ’―
  • rethink our current documentation website πŸ“–
  • does everyone in the Matrix team know how to use TypeScript? πŸ‘©β€πŸ’»
  • publishing / compiling the SDK πŸ“˜
  • testing PR's using a CI πŸ€–
  • what do you think about using TypeScript? πŸ€”

I think we can all say that TypeScript makes for a substantially improved developer experience. Ranging from auto-suggested methods, properties and functions, the use of TypeScript makes a library so much easier to use: in most cases, understanding the library isn't actually needed. Auto-suggestions can empower developers to learn a library on-the-go, without requiring the use of complex documentation websites.

TypeScript is the way forward, and the developer community have continuously established this fact: major libraries have quickly gained support for TypeScript. Developers favour TypeScript because it has, in many respects, greatly simplified the use and deployment of JavaScript. πŸ‘Œ

Where We're At

Without Element, Matrix may not be where it is today. In this respect, introducing TypeScript would substantially reduce the burden on developers who rely on the JavaScript SDK, including the developers of Element.

I speak from experience when I say that Matrix JavaScript SDK has definitely not been easy to use. Many a time I've had to resort to asking questions in the Developer room, placing more burden on overworked developers in the midst of a pandemic.

I'd like to spend time improving upon that, which will ultimately benefit everyone.

This will allow developers to use our SDK without reading ghastly, non-user-friendly, web-pages of documentation. Instead of this outdated approach, we can read documentation without leaving the editor, saving hours of understanding the internals of our SDK.

We can do better! πŸ˜„

It's Time to Improve

That's why, with your support, I'd like to finally bring TypeScript into our SDK. Instead of relying on other developers to maintain types, we can just do it ourselves! πŸŽ‰

Behind the scenes, I've spent hours working on native types for our SDK. This will make life easier for everyone: consumers of the SDK, maintainers of the external types, the Element developers, and newcomers who wish to extend Element. πŸ’―

What Do You Think?

I'd love to hear what you think. If TypeScript isn't a good fit, that's OK, but it's a proposal we definitely need to consider. I've already outlined the numerous benefits of using TypeScript above. I think it's a good fit for this library, but I welcome feedback that suggests the contrary!

My thoughts on the matter: we already use TypeScript quite heavily in our React SDK, and it may be good to unify that with the JS SDK. TypeScript just makes things... easier.

I Need Your Help

However, I've encountered a few problems (and questions ❓), which are enumerated below:

"Can I convert MatrixBaseApis from a mixin into a class?"

As TypeScript doesn't really support mixins without substantial type hackery, it may be wise to convert this to a class. In that vein, we would have to extend MatrixBaseApis with the EventEmitter interface.

utils.inherit isn't type-safe, and IMO, is a workaround for forcing multi-inheritance in a language which doesn't natively support it. Even the Node developers discourage use of util.inherits. πŸ‘Ž

We'll need to rethink our API Reference website.

Right now, we use JSDoc for documentation, which doesn't support TypeScript. There are alternatives such as TypeDoc, but I haven't had a good experience using that, so I'd recommend against it. πŸ‘Ž

If we do move to TypeScript, we'll need a generator which supports it. We could either continue to maintain both an API Reference and TypeScript typings, or adopt React's style of documentation, which includes easy-to-understand guides for using their library.

This may be a good time to rethink our documentation. In the past, I have proposed a set of guides for using this SDK, which I would be open to writing myself. The style of React's documentation is very easy to comprehend, with a clear navigational structure, unlike a classical API Reference (which is an approach many libraries now avoid altogether).

Should we also include Flow typings?

I'm slightly against it πŸ‘Ž, but figure I may as well ask: **would it be worth using a TypeScript β†’ Flow generator, to cater to Flow users?

Do we care about Flow users? πŸ’­

Do all core Matrix developers know how to use TypeScript?

If we do end up transitioning to TypeScript, it's important to note whether or note the core Matrix developers actually know how to use it. I wouldn't want to exclude anyone from developing the SDK πŸ˜…, despite the major benefits of TypeScript.

How are we going to set up the CI?

TypeScript is really useful when used in a CI, so we can automatically check whether or not PR's are wrongly typed (or contain errors). In theory, this should be relatively easy to setup: our very own Matrix React SDK contains a CI for checking TypeScript, so it should be a matter of transferring the configuration over to the JS SDK. πŸ‘Œ

Publishing TypeScript

Same as above, we've already set this up, but seems like a good point to note. We'll definitely need a script to do this... πŸ€”

(This list may be updated in the future.)

@gbaranski
Copy link

Awesome idea, I'm getting headache after struggling with any almost everywhere, and that I need to import library like that

/* eslint-disable @typescript-eslint/ban-ts-comment */
import _matrix from 'matrix-js-sdk';
// @ts-ignore
import { EventStatus, LoginPayload, MatrixEvent } from '@types/matrix-js-sdk';
// @ts-ignore
const matrix: typeof import('@types/matrix-js-sdk') = _matrix;

@resynth1943
Copy link
Author

It's a good idea, but to be honest I'm not sure if I want to finish it due to me taking a long break from this platform.

I've already done a substantial amount of the work, which will be uploaded shortly.

@resynth1943
Copy link
Author

Just a quick update on where I'm at: I'm not really sure how any of the Matrix developers feel about this, as they haven't left one comment or reaction in over a week.

I'm therefore halting my contributions forthwith, as I don't wish to spend any more hours working on something that might get closed when someone looks at it.

I will definitely upload the substantial amount of work I've already done to the relevant repository.

@gbaranski perhaps you could continue the work? I won't blame you if you say no.

@gbaranski
Copy link

@resynth1943 that might be great opportunity to learn how this works.

Can any of the matrix-js-sdk developers share their opinion about it?

@resynth1943
Copy link
Author

resynth1943 commented Dec 4, 2020

Hi @gbaranski, I've updated the code here.

Just to be clear, I don't wish to offend the Matrix developers, but quite frankly I'm not sure whether I should be spending more time on this; as I've seen no response, I'm honestly not sure if the developers are skeptical about this, or if there's a general consensus that this is a good proposal.

I don't want to dedicate hours of my time to something that could end up being refuted; in that respect, I could be working on my new framework to automate the operation of my computer, and that seems like a wise idea in the middle of a global pandemic (especially since no public body seems to know what they're doing).

Anyway, for anyone who wishes to take over, here are some notes about my refactored code:

  • Just so you're aware, the MatrixClient class is totally incomplete and broken (with a substantial amount of syntax errors). Whoever touches that may wish to redo the whole thing, but my code is a pretty solid base. I've replaced unknown types (which should be replaced with an actual type) with UNKNOWN_TYPE_FILL_ME_IN_LATER, so a quick Ctrl+F for that will show up what I haven't done.

  • There is also a lot of broken syntax in the MatrixClient, mainly due to me copy-pasting the MatrixClient.prototype.[...] = [...] declarations directly into the class, then removing the .prototype nonsense with a regular expression. Should be relatively easy to solve these, however, as the errors seem to lie in un-needed { characters and semi-colons at the end of each method declaration.

  • In the MatrixClient file, IClientOpts is actually a duplicate of the ICreateClientOpts type in src/matrix.ts (not my code). However, it's probably a wise idea to merge the two instead of dropping mine, since I've gone through the effort of adding documentation to my interface declaration.

  • Furthermore, there is still quite a substantial amount of untyped JS (sometimes with the accompanying JSDoc types) in MatrixClient. I have converted quite a lot of JSDoc to TypeScript, so it may be a good place to start.

  • Additionally, I haven't actually introduced strongly-typed emitters into MatrixClient, so a .on(... call won't yet yield any typed suggestions from the Language Server. The events are somewhat documented in the JS SDK's documentation website, however most of it's marked as TODO...

  • A substantial amount of code lies in the MatrixClient, so that's probably where you want to start; I had started working on that prior to submitting this proposal, so a good deal of work has already been done (but there are syntax errors -- beware!)

  • Converting a JSDoc-annotated method / property is just a matter of transferring the types from the JSDoc to the TypeScript, then eliminating the JSDoc types altogether (but not the documentation they contain, of course).

  • There are some undocumented methods, and the official documentation does no favours in that respect; a large majority of items are marked as TODO, so you'll definitely have to dig into the code-base and figure out the types yourself.

  • I don't believe there's a package containing types for Matrix server-to-client / client-to-server responses (which are documented with Swagger). There are various mature implementations for converting Swagger into generated TypeScript typings, but I don't believe anyone (official) has taken up that opportunity insofar. If you wish to continue this, I'd seriously recommend taking this opportunity up.

  • Further to the last point: instead of (manually) re-defining Swagger types in the JS SDK, consider using an external package to host said types. This keeps bloat out (it's a little late for that, but do what you can) of the SDK.

  • This is slightly off-topic, but Matrix JS SDK seems to use the deprecated "request" module. This needs to be replaced with a stable implementation immediately.

  • Please remember to transfer any and all (non-existing) JSDoc textual documentation to the code-base. Of course, remove the types (e.g. {number}), but the documentation needs to remain in the source code. Do not remove the whole JSDoc documentation, only the types. See an example of this here.

  • Feel free to create a new PR, rebased onto my branch, which incorporates your changes. As I have no further plans to work on this, I would be ever so grateful if someone else could take up the reins. This is best for the Matrix developer community as a whole, as outlined in the proposal above.

  • To continue this transition, you're probably best off using VSCode, or a derivative such as VSCodium. These editors have the best support for TypeScript, and will definitely make things easier for you.

Thanks again,
Resynth

@jryans
Copy link
Collaborator

jryans commented Dec 11, 2020

We're already using TypeScript in various places, so continuing that conversion seems like a good idea. I would recommend small PRs for anyone interesting in converting bits at a time.

There are lots of interesting ideas here, but it's hard to work with as single massive issue, because each part would complete at different times, so they should be in separate issues. Instead of a massive issue like this, it might be more practical to discuss with the team and work on a small piece instead.

@jryans jryans closed this as completed Dec 11, 2020
@resynth1943
Copy link
Author

@jryans thank you for your response.

I wasn't sure whether you actually wanted to go forward with this, because
there wasn't any official response for 16 days. Thank you for telling me where
you're at with this.

After discussing it with a team member, they recommended
I create an official proposal, hence the 'single massive issue' you see here!

I won't be working on this, but I've outlined what needs
to be done above, in small bite-size notes.

Best of luck!

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

No branches or pull requests

3 participants