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

Convert library code to TypeScript #762

Merged
merged 307 commits into from
Feb 25, 2022
Merged

Convert library code to TypeScript #762

merged 307 commits into from
Feb 25, 2022

Conversation

owenpearson
Copy link
Member

@owenpearson owenpearson commented May 17, 2021

Resolves #686

  • Converts the library code to TypeScript. I've also opted to rewrite the code using ES6+ syntax which is compiled down to ES3 using the TypeScript compiler.
  • I've used a few generally undesirable TypeScript features here in the interest of being able to get this change landed sooner (in particular I've used the any type quite widely and there are quite a few explicit type casts). We don't need to keep a note of these since they can easily be identified using ESLint and I do intend to fix them in the near future.
  • The bundle size for ably.min.js (the minified browser bundle) has increased somewhat (from 193.5kb to 222kb). I don't think there's too much we can do about this right now without a lot of effort/risk, and I think there are better things we can do to reduce bundle size for lib consumers in future (like making the library more compatible with tree shaking).

Note for reviewers: All of the changes in this branch have been reviewed independently so don't feel it necessary to read through all of the changes included here; I'm really just looking for more general feedback on the overall structure of what's being changed here (although feel free to comment on specific library code changes if you want).

@github-actions github-actions bot temporarily deployed to staging/pull/762/bundle-report September 30, 2021 11:32 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/762/bundle-report October 4, 2021 14:08 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/762/bundle-report October 4, 2021 14:13 Inactive
fix retry timer setting wrong variable to null (Fixes #898)
@github-actions github-actions bot temporarily deployed to staging/pull/762/bundle-report February 24, 2022 13:32 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/762/bundle-report February 24, 2022 17:02 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/762/bundle-report February 25, 2022 11:11 Inactive
Copy link
Contributor

@QuintinWillison QuintinWillison left a comment

Choose a reason for hiding this comment

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

I've pretty much skim read thru, given we know that all of this work has already been reviewed once in upstream PRs.

Just a few things that I think could still have a tidy, or I've only just spotted...

ably.d.ts Show resolved Hide resolved
browser/fragments/platform-reactnative.ts Outdated Show resolved Hide resolved
browser/lib/transport/xhrrequest.ts Outdated Show resolved Hide resolved
browser/lib/util/http.ts Outdated Show resolved Hide resolved
common/lib/client/rest.ts Show resolved Hide resolved
Copy link
Member

@lmars lmars left a comment

Choose a reason for hiding this comment

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

Epic effort! LGTM

@owenpearson owenpearson merged commit f5416c5 into main Feb 25, 2022
@owenpearson owenpearson deleted the integration/typescript branch February 25, 2022 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Migrate code base to typescript
5 participants