Convert source code to TypeScript

[raucao]

I think this will make the source code easier to understand and reason about, especially in the very large classes/modules that pass data between many functions. It also removes the potential for type-related bugs, of course. Last but not least, with the popularity of TypeScript, people have already asked for TS definitions for this library, for importing it in their TS codebases.

As of now, I have only converted the Sync class, because that was the one where I was trying to debug and change something at the time. I haven't even started adding types really, but already fixed a bunch of other issues that the TS compiler complained about.

@johannesjo has offered to help with this, and proposed a mode of collaboration:

We probably should take good care not to work on it at the same time, as there are so many potential conflicts. Maybe we can do it like this: Once we start working we write a comment on the Draft PR and once we’re done (no unpushed local changes) we write another one?

I like the idea of writing comments with human-readable updates when handing back the code. But I think for managing who's currently working on it, we can just use the assignee feature. Whoever starts on something just self-assigns the PR, and unassigns when they stop pushing changes.

@johannesjo, I just invited you as a collaborator to the RS org, so you can push directly to this branch and also self-assign PRs and issues in any of the org repos.

To Do

• Convert all source files to TypeScript files (with no compilation/ESLint errors, but warnings allowed)
• Lint TypeScript (instead of JS)
• Update release/versioning process/scripts to include TS compilation
• Implement process for publishing type definitions
• Fix/update node.js usage/integration (index.js)
• Ensure the documentation setup still works with the new source files (Sphinx + JSDoc)

[raucao]

I added some more types and re-added esformatter. The idea is that we don't commit the JS files for now, until we can get the formatting similar enough to what it was before, in order to preserve git-blame-able history.

(I actually do have 2 failing tests for the new SyncError definition locally, but Travis is still testing the old JS code.)

[johannesjo]

@skddc maybe it makes sense to get rid of the JavaScript files completely in favor of a single one? Otherwise we might run into the risk of people submitting PRs with changes to them instead of the typescript files.

Or are they needed for backwards compatibility?

[raucao]

I would really like to preserve the history. It's helping a lot with this codebase, especially because most of the Sync code e.g. wasn't written by an active maintainer. It's also currently completely tied in with the test suite, but maybe we can change that.

[johannesjo]

Makes sense! One step after another :)

[johannesjo]

I tried migrating the access.js to typescript there I came across this line:

remotestorage.js/src/access.js

Line 53 in 61cbbb2

this.set(name, savedMap[name]);

I wonder where is set coming from? Will this be assigned from the outside? Or is the implementation missing?

[johannesjo]

I also just recognized that I am not allowed to push to this branch. Should I just create a PR to to it?

[raucao]

I also just recognized that I am not allowed to push to this branch. Should I just create a PR to to it?

No, you should definitely have push access. Let me check again.

Edit: you should have push access already. If not, then GitHub is broken. I changed the access for that team before I opened the PR. I'll add you to another team as well, which had push access before (it's just an old "collaborators" vs "contributors" team, there's no actual difference).

[raucao]

I wonder where is set coming from? Will this be assigned from the outside? Or is the implementation missing?

Wow, did you just find a glaring bug in pretty much the only function that's not tested there? I don't see where it could come from.

[raucao]

I looked at this, and I think the reason the test is missing and there's a bug in the function is that it is neither a public API, nor used anywhere else in the code AFAICS.

I also don't see a use case in which we would ever remove a scope there. So, I think we can remove it entirely.

[johannesjo]

In this case I just comment it out for now. Maybe we can deal with this in a separate issue.

Not sure if I am doing something wrong regarding the access. I just cloned the repo and tried pushing to this branch directly with my standard credentials. Not sure if that's the right way. I am new to how organizations work on github.

[raucao]

Could you post the origin URL that you added for the remote? And the command you use for pushing?

[johannesjo]

[remote "origin"]
	url = https://github.com/remotestorage/remotestorage.js.git
	fetch = +refs/heads/*:refs/remotes/origin/*
[branch "master"]
	remote = origin
	merge = refs/heads/master
[branch "feature/typescript_conversion"]
	remote = origin
	merge = refs/heads/feature/typescript_conversion

And the git command:

git push  origin feature/typescript_conversion:feature/typescript_conversion

Git then asks me for my credentials, but entering leads to authentication failure.

[raucao]

Oh wait, could it be due to the draft status?

[raucao]

Git then asks me for my credentials, but entering leads to authentication failure.

Authentication failure is between you and GitHub I believe. Shouldn't have anything to do with this repo.

[johannesjo]

I should add that the authentication failure is only for this repo. Otherwise it works fine (via public key). Just tried pushing again, no change.

[raucao]

I should add that the authentication failure is only for this repo

Ah, got it. Thanks.

Just tried pushing again, no change.

Damn. You're in two different teams that both have push access. Looks like it's really a bug in GitHub then. Perhaps some overloaded worker queues or something. :/

You could do a PR against this branch for now, and if it still doesn't work tomorrow, I can contact GitHub's support.

Edit: trying one last thing. In addition to team memberships I also just added your account as a separate entity to the access list.

[raucao]

So, for the last couple of days I wrestled with sphinx-js, because for some reason it can't find functions via autofunction anymore. I tried it with @silverbucket, too, and it was the same on his machine (with different OS and Sphinx 2.8.x instead of 3.0.x).

Until today, I thought this only happened since I switched from JSDoc to TypeDoc, but I just double-checked the current master, and the exact same thing actually happens for our current JSDoc setup as well. :/

The errors look like this:

No JSDoc documentation was found for object "Access#claim" or any path ending with that

Unfortunately, there's no backtrace, or anything else that would help with debugging. I also tried using path syntax, instead of the capitalized class name syntax for finding the function, but no dice. And autoclass also doesn't find anything. If anyone has experience with Sphinx or Python, any help would be appreciated. (From current sphinx-js issues, it doesn't look like it's a common problem for other users.)

[raucao]

Would be great if some people could review/test the current state of this branch (minus the docs). Thanks!

[galfert]

I can confirm that the test suite is green for me locally. So far I didn't have much time to have a deeper look at this PR, but I hope I will get to it in the next few days.

[galfert]

Wow, looks like a lot of work went into this. Great job.

I noticed some formatting issues. Mostly missing spaces after an if. I created a separate commit for fixing those, because commenting on every one of them was getting too cumbersome :)

Other than that, I left a comment about a change in the feature initialization.

[raucao]

Status update:

Notwithstanding the fact that we saw the same bug in Sphinx 2.8, the developers have released a version of sphinx-js that is officially compatible with Sphinx 3 in the meantime. There are also a bunch of open PRs on the sphinx-js repo, some of which sound relevant to the bug that we're seeing.

I pushed the updated version, as well as the TypeDoc config and contributing doc to this branch now. Unfortunately, I'm now seeing a different bug that prevents building the docs:

Extension error:
Handler <function gather_doclets at 0x7f0d9d2519d0> for event 'builder-inited' threw an exception

I have added this to the issue I opened over there. Let's see if someone knows what it could be.

---

On a side note, they're also working on decoupling sphinx-js from JSDoc more, so that it's possible to add more TypeScript (and other) features soon. Sounds good to me.

[raucao]

Thinking about this some more, I changed my opinion about the status of this PR.

How about we merge it as is, and document the remaining tasks in separate issues? This way we can start refactoring the TS code at the same time as resolving the sphinx-js bugs and preparing the final release process. And we can better split those tasks as well.

We would just not be able to tag a release from master until the blockers have been resolved. But we already have a blocker label for this purpose. (Edit: we still could/should prepare one or more release candidate tags from master before the final release then.)

What do you think?

/cc @remotestorage/core

[galfert]

I don't have any objections to this.

[raucao]

Thanks.

Just a quick status update: After fixing some last bugs I found with the actual release build, I'm currently in JavaScript module hell, trying to get the release and test suite to work at the same time. I have switched building to be all done by Webpack directly, so we also get TS stacktraces in browsers even. But I have to figure out some last things before I can push my commits for this.

[raucao]

Alright, I have added new issues for the tasks that remain before we can tag a release after merging this. I have also added a milestone for it:

https://github.com/remotestorage/remotestorage.js/milestone/16

Any and all help with testing, as well as adding, commenting on, or solving issues in that milestone, would be most appreciated, of course.