Convert from Flow to Typescript #816

FredKSchott · 2017-12-04T19:50:04Z

TypeScript is very similar to Flow, in that both provide a flexible typing system for JavaScript. The syntax is almost identical, so it should feel perfectly comfortable already. But TypeScript is much more powerful, so with the same syntax you catch more errors and enforce clearer code. This is a good, fair comparison of the two, and here are some examples in rippled-lib itself:

Simple bad typing not caught by Flow
Broken (or undocumented/deprecated) call to lodash
Broken call to Error.captureStackTrace
You can search this PR for "TODO:" to see other bugs I highlighted but couldn't fix.

Additionally it's growing faster, adding new features, and is already fairly popular in the larger npm ecosystem. Lodash, for ex, publishes TS type definitions directly in their package, which is how I was able to catch that error above automatically. Also, type checking is a part of your build/compile, so they can't get out of date with the code.

This allows us to add function typing information to the new, main request() interface. That way all consumers, even those using regular JavaScript with an IDE like VSCode, will get typing help when making API requests and handling API responses. See #812 for more information.

/cc @intelliot This touches a lot of code so please ask if anything is confusing / looks off / could use some documentation.

FredKSchott · 2017-12-04T19:52:18Z

package.json

@@ -64,11 +60,11 @@
    "docgen": "node --harmony scripts/build_docs.js",
    "clean": "rm -rf dist/npm",
    "typecheck": "flow check",
-    "compile": "babel -D --optional runtime -d dist/npm/ src/",
-    "watch": "babel -w -D --optional runtime -d dist/npm/ src/",
+    "compile": "mkdir -p dist/npm/common && cp -r src/common/schemas dist/npm/common/ && tsc",


typescript only handles typescript files, so we have to copy the schemas directory here ourselves. Normally you could just move this folder to a single place outside of the src directory and load them by relative path, but unfortunately that breaks because dist/npm & src/ are at different levels of nesting (../ vs ../../).

This doesn't feel too complex but if we ever need to add more here we could move this into a build script or gulp file.

FredKSchott · 2017-12-04T19:53:58Z

src/api.ts

-  getServerInfo,
-  getFee,
-  getLedgerVersion,
+  connect = connect


Adding these as class properties makes them a part of the interface, vs. _.assign() which was appending them to the prototype in a way that confused TypeScript.

intelliot · 2017-12-07T18:44:42Z

src/broadcast.ts

@@ -3,11 +3,15 @@ import * as _ from 'lodash'
 import {RippleAPI} from './api'

 class RippleAPIBroadcast extends RippleAPI {
+
+  // TODO: Should this default to 0, or null/undefined?


I think 0 is good here.

> 1 > undefined false

@intelliot my concern is that 0 is a number but isn't a valid ledger number. It do mean that things like (ledger.ledgerVersion > this.ledgerVersion) will work by default, but it also means we have to document and explain that 0 = unset/unknown. It would be more explicit if ledgerVersion = undefined means it hasn't been looked up yet.

That's a good point. I'm fine with undefined, but make sure to update the if condition (ledger.ledgerVersion > this.ledgerVersion) too.

intelliot · 2017-12-07T18:47:08Z

src/common/connection.ts

+  private _proxyAuthorization?: string
+  private _authorization?: string
+  private _trustedCertificates?: string[]
+  // TODO: options.key never used?


Looks like it's used in _createWebSocket()

Ha, can't believe I missed that

intelliot · 2017-12-07T18:51:56Z

Still looking at this. Hoping to try it out soon :)

Any potentially breaking changes?

AGallouin · 2017-12-15T10:03:54Z

Any news on this ? I'm really looking forward to this !

sublimator · 2017-12-15T10:09:53Z

They too busy celebrating? haha

intelliot

This is a great start. I left comments on a few of the TODOs.

Also, ripple-lib can be used with either node or in the browser, so I think we need to make some changes to our webpack config for browser builds to work.

intelliot · 2017-12-07T18:50:36Z

src/common/connection.ts

@@ -186,7 +212,8 @@ class Connection extends events.EventEmitter {

      this._retry = 0
      this._ws.on('error', error => {
-        if (process.browser && error && error.type === 'error') {
+        // TODO: "type" does not exist on official error type, safe to remove?


I'm not sure. Perhaps some versions of ws included it? To be safe, we could remove it in a later release and note it in the changelog.

sounds good, I'll just leave it as a TODO then

intelliot · 2017-12-16T04:05:01Z

src/common/serverinfo.ts

@@ -60,16 +60,19 @@ function getServerInfo(connection: Connection): Promise<GetServerInfoResponse> {
  })
 }

+// TODO: This was originally annotated to return a number, but actually
+// returned a toString'ed number. Should this actually be returning a number?


I believe this should be a string. We generally use strings for all currency amounts.

sounds good.

Separately: Looking at this more closely now, should this use BigNumber internally? Number could cause problems with baseFeeXRP being a float, right?

Yes, it would be correct to use BigNumber here internally.

intelliot · 2017-12-16T04:10:32Z

src/ledger/pathfind.ts

+    paths.alternatives = _.filter(paths.alternatives, alt =>
+        !!alt.source_amount &&
+        !!pathfind.source.amount &&
+        // TODO: Returns false when alt.source_amount is a string. Fix?


How can alt.source_amount be a string?

alt.source_amount is of type RippledAmount, which can be a string. https://ripple.com/build/rippled-apis/ seems to document a few places where source_amount is a string.

Looks like source_amount is only a string when the currency is XRP, in which case there is no need for pathfinding. So I think it's fine to return false in that case.

Looks like source_amount is only a string when the currency is XRP, in which case there is no need for pathfinding.

You can send XRP and use paths to send the recipient USD ...

What you mean no need for path finding ?

intelliot · 2017-12-16T04:11:42Z

src/server/server.ts

@@ -23,12 +21,14 @@ function getServerInfo(): Promise<GetServerInfoResponse> {
  return common.serverInfo.getServerInfo(this.connection)
 }

-function getFee(): Promise<number> {
+// TODO: getFee() was originally annotated to return a number, but actually
+// returned a toString'ed number. Should this actually be returning a number?


This should return a string.

intelliot · 2017-12-16T04:12:51Z

src/transaction/combine.ts

@@ -21,7 +19,9 @@ function compareSigners(a, b) {
 function combine(signedTransactions: Array<string>): Object {
  validate.combine({signedTransactions})

-  const txs = _.map(signedTransactions, binary.decode)
+  // TODO: signedTransactions is an array of strings in the documentation, but
+  // tests and this code handle it as an array of objects. Fix!


Good catch 🤔

FredKSchott · 2017-12-16T08:26:33Z

@intelliot Great catch re: webpack. I'll get that fixed for this PR.

Thanks for the review! No behavior should be changing here, except for a few small bug fixes that were explicitly bugs. Anything that could have been relied on as behavior was left as-is with a TODO, so no breaking changes expected.

Excited to do some more work on this over the holidays. Three branches can go into PR all together once this gets merged:

istanbul update Istanbul is now called "nyc", works with typescript automatically
tslint like eslint but for typescript, rules ported over from the eslintrc config
beginning to flesh out new request api: so far added support for account_offers, book_offers, and account_info. Doesn't need to be merged into "develop" branch just yet, but an early review would be good to test out & make sure it's going in the right direction.

FredKSchott · 2017-12-16T21:30:23Z

webpack & gulp updated for TS: FredKSchott/ripple-lib@typescript...FredKSchott:webpack

It's currently in a separate branch, but I can add it directly into this PR if you'd prefer.

intelliot · 2017-12-19T01:43:39Z

That looks great!

I attempted to create a PR and merge that branch to this PR, but github won't allow it since I don't have write access to your fork 😅

FredKSchott · 2017-12-20T19:41:24Z

Haha np, I'll merge it in now

Update webpack, gulpfile to support typescript

intelliot

This looks good. The remaining TODOs could be handled later. I'll do a bit more manual testing meanwhile.

intelliot · 2017-12-24T08:48:50Z

Squashed and merged! I don't think there will be any difficulty rebasing on the squash commit, but let me know if so.

FredKSchott · 2018-01-02T18:32:23Z

@intelliot woo! 🎉 Thanks for your reviews

Just getting back & caught up from the holidays, I'll rebase those other branches and create the next batch of PRs

Fred K. Schott added 2 commits December 4, 2017 11:36

convert to typescript :bang:

451b0d4

add docs for custom node typing

db2d7bf

FredKSchott commented Dec 4, 2017

View reviewed changes

fix bad fixture change

507bc7a

intelliot reviewed Dec 7, 2017

View reviewed changes

intelliot requested changes Dec 16, 2017

View reviewed changes

remove handled todos

d5dadf0

FredKSchott mentioned this pull request Dec 16, 2017

Proposal: Simplified Requests, Flexible Responses #812

Closed

fix bad wswrapper definition

47bdba2

Update webpack, gulpfile to support typescript

b6a2de1

Merge pull request #1 from FredKSchott/webpack

0149acc

Update webpack, gulpfile to support typescript

intelliot reviewed Dec 20, 2017

View reviewed changes

intelliot approved these changes Dec 22, 2017

View reviewed changes

intelliot merged commit 8204f6c into XRPLF:develop Dec 24, 2017

mkreidenweis mentioned this pull request Jan 4, 2018

Add TypeScript linting #827

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert from Flow to Typescript #816

Convert from Flow to Typescript #816

FredKSchott commented Dec 4, 2017 •

edited

Loading

FredKSchott Dec 4, 2017

FredKSchott Dec 4, 2017

intelliot Dec 7, 2017

sublimator Dec 15, 2017

FredKSchott Dec 16, 2017

intelliot Dec 20, 2017

intelliot Dec 7, 2017

FredKSchott Dec 16, 2017

intelliot commented Dec 7, 2017

AGallouin commented Dec 15, 2017

sublimator commented Dec 15, 2017

intelliot left a comment

intelliot Dec 7, 2017

FredKSchott Dec 16, 2017

intelliot Dec 16, 2017

FredKSchott Dec 16, 2017 •

edited

Loading

intelliot Dec 20, 2017

intelliot Dec 16, 2017

FredKSchott Dec 16, 2017

intelliot Dec 20, 2017

sublimator Dec 21, 2017

intelliot Dec 16, 2017

intelliot Dec 16, 2017

FredKSchott commented Dec 16, 2017 •

edited by intelliot

Loading

FredKSchott commented Dec 16, 2017

intelliot commented Dec 19, 2017

FredKSchott commented Dec 20, 2017

intelliot left a comment

intelliot commented Dec 24, 2017

FredKSchott commented Jan 2, 2018 •

edited

Loading

Convert from Flow to Typescript #816

Convert from Flow to Typescript #816

Conversation

FredKSchott commented Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

intelliot commented Dec 7, 2017

AGallouin commented Dec 15, 2017

sublimator commented Dec 15, 2017

intelliot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FredKSchott Dec 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FredKSchott commented Dec 16, 2017 • edited by intelliot Loading

FredKSchott commented Dec 16, 2017

intelliot commented Dec 19, 2017

FredKSchott commented Dec 20, 2017

intelliot left a comment

Choose a reason for hiding this comment

intelliot commented Dec 24, 2017

FredKSchott commented Jan 2, 2018 • edited Loading

FredKSchott commented Dec 4, 2017 •

edited

Loading

FredKSchott Dec 16, 2017 •

edited

Loading

FredKSchott commented Dec 16, 2017 •

edited by intelliot

Loading

FredKSchott commented Jan 2, 2018 •

edited

Loading