Translate source code to typescript #197

mzgoddard · 2020-07-28T16:59:18Z

Use grunt-ts to transpile typescript to /lib directory for publishing to npm
Translate function syntax classes and commonjs modules to class syntax classes and es6 modules
Add types to public interfaces
- Add types to parameters for Query class
- Add overloaded method types to bound methods with callbackToPromise

rwaldron

First review done! Looks great!

rwaldron · 2020-08-05T18:07:00Z

src/airtable.ts

+    readonly _endpointUrl: string;
+    readonly _apiVersion: string;
+    readonly _apiVersionMajor: string;
+    readonly _noRetryIfRateLimited: boolean;


Is there a legacy requirement to keep these named with _? This seems like a good use case for private fields, but I can understand not using them if some other code somewhere else is somehow relying on them.

One can argue otherwise but I consider that a backwards incompatible change. I suppose it might be a minor breaking change instead of major breaking change. But I want to avoid any mechanical change in this changeset.

Beyond this PR, I agree, switching to private fields would be good here.

we should mark these as private, but our style guide still encourages prefixing private methods with _

Marking these as private creates typescript errors in base.ts.

Property '_endpointUrl' is private and only accessible within class 'Airtable'. ts(2341)

So we can't mark these properties in airtable.ts as private without changing the executed relationship in base.ts. That probably means some kind of API refactor?

sounds good - we can leave these as-is for now and maybe address in a follow-up

rwaldron · 2020-08-05T18:08:25Z

src/airtable.ts

+                    Airtable.noRetryIfRateLimited ||
+                    defaultConfig.noRetryIfRateLimited,
+            },
+        });


Continuing from my last question, if you chose to use private fields, this would be simplified.

rwaldron · 2020-08-05T18:09:47Z

src/airtable.ts

+            apiVersion: '0.1.0',
+            apiKey: process.env.AIRTABLE_API_KEY,
+            noRetryIfRateLimited: false,
+            requestTimeout: 300 * 1000, // 5 minutes


Does TS support numeric separators? Asking for a friend. 300_000 ;)

Hmm. Not sure. If I had changed this line (instead of the automated changes made by lebab and prettier) I'd probably make this 5 * 60 * 1000. Or change the line to 300_000, // 300,000 milliseconds (5 minutes) with your recommendation.

rwaldron · 2020-08-05T18:12:56Z

src/airtable_error.ts

+            this.error,
+            ')',
+            this.statusCode ? `[Http code ${this.statusCode}]` : '',
+        ].join('');


`${this.message} (${this.error}) ${this.statusCode ? `[Http code ${this.statusCode}]` : ''}`;

?

I had that same thought when I was making changes. But I don't want to change behaviour in this PR. (I really wanted to change the fetch inverted promise chain in another file.)

rwaldron · 2020-08-05T18:14:10Z

src/airtable_error.ts

+    message;
+    statusCode;
+
+    constructor(error, message, statusCode) {


Is there are legacy reason for this parameter list?

rwaldron · 2020-08-06T00:49:23Z

src/query.ts

+        throw new Error('The first parameter to `eachPage` must be a function');
+    }
+
+    if (!isFunction(done) && done !== void 0) {


This has the same behavior as the parameter I pointed out previously.

rwaldron · 2020-08-06T00:50:02Z

src/query.ts

+        throw new Error('The second parameter to `eachPage` must be a function or undefined');
+    }
+
+    const that = this;


Unnecessary when all occurrences of this are in arrow functions

rwaldron · 2020-08-06T00:55:04Z

src/run_action.ts

-var AbortController = require('./abort-controller');
-
-var userAgent = 'Airtable.js/' + packageVersion;
+const userAgent = `Airtable.js/${packageVersion}`;

 function runAction(base, method, path, queryParams, bodyData, callback, numAttempts) {


Can this be an async function?

rwaldron · 2020-08-06T00:55:11Z

src/run_action.ts

@@ -1,31 +1,23 @@
-'use strict';
+ import exponentialBackoffWithJitter from './exponential_backoff_with_jitter';


How'd that get there?

rwaldron · 2020-08-06T00:57:37Z

src/typecheck.ts

@@ -0,0 +1,24 @@
+import includes from 'lodash/includes';
+import isArray from 'lodash/isArray';


Array.isArray

mzgoddard · 2020-08-10T20:20:07Z

Rebased to fixup the gitignore conflict.

SomeHats

Thanks so much for this! And thanks @rwaldron for the review too.

Generally this looks great! I've left a few comments and questions particularly on areas where we can make changes with confidence that we're not changing runtime semantics at all. For the other refactors & cleanups (inc. reducing our reliance on lodash where built-ins would suffice), i've left some suggestions but would rather we handle those in a follow-up PR as this diff is already pretty large.

package.json

src/airtable_error.ts

SomeHats · 2020-08-04T13:35:06Z

src/base.ts

+    readonly _airtable: Airtable;
+    readonly _id: string;


could these be private?

Making them private produces a typescript error because code outside of the class modifies these values currently.

Ah, wait. No. That's another file I'm thinking about. We can make these properties private.

So we can't mark the _ properties in airtable.ts private without some kind of refactor in airtable.ts and base.ts.

#197 (comment)

But we can mark the _ prefixed properties of base.ts private without changing how any js executes. Personally I agree we should mark the members in base.ts as private, however even without changing any executing source, I think that will make this change a major api breaking change for users using typescript and airtable.js if they are also accessing those members already. This seems like a small detail but should we add private to these members as a part of the follow ups?

given we don't currently provide typescript types, i don't know that just marking them private constitutes a breaking change. happy to leave as-is and address in a follow-up though!

happy to leave as-is and address in a follow-up though!

That's my thought.

SomeHats · 2020-08-17T14:11:24Z

src/base.ts

+
+        options = options || {};
+
+        const method = get(options, 'method', 'GET').toUpperCase();


potential follow-up: Do we still need lodash's get here? Would options.method ?? 'GET' work instead? ditto for other places i see it used in this project

SomeHats · 2020-08-17T14:19:19Z

tsconfig.json

@@ -0,0 +1,13 @@
+{


could we specify "lib": "ES5" in here too? right now, we this includes dom types, which might not be available in some peoples typescript projects, and i'm worried that us referencing things like RequestInit could cause type issues in other people's projects

Hmm. We'll need to add import 'typescript/lib/lib.dom'; to any files in these packages using fetch types.

I'm not sure if thats the right way to do that. That may import dom types globally for all files. As in it may pollute the files of the user's files that are depending on airtable.js.

let's leave as-is for now, we can look at explicitly importing the types we need for fetch in a follow-up. i'm not even sure this'll cause problems tbh

SomeHats · 2020-08-17T14:37:49Z

src/fetch.ts

@@ -0,0 +1,6 @@
+import nodeFetch from 'node-fetch';


potential follow-up: what it we replaced this file with isomorphic-fetch?

@SomeHats while doing due diligence on this, I note that:

isomorphic-fetch:

was last published 5 years ago

has > 4.5M downloads per week

16 open PRs (the newest is over a year old)

61 open issues and the newest is "Is this library still been maintained?" 🤣

node-fetch

was last published 22 days ago

has >16M downloads per week

PR queue is active

Issue queue is active

I don't think it's in Airtable.js's best interest to migrate to what appears to be an unmaintained package, so I'd like to close this "won't fix"

fwiw, isomorphic-fetch just re-exports node-fetch (in node) or a polyfill/browser-builtins (in the browser). node-fetch actually references isomorphic-fetch (and another maybe more suitable option) in its docs. i'm not fussed about the specific thing we use here though - the main thing i'm interested in is that for browser users, we don't include all of node-fetch in the bundle unnecessarily.

Gotcha! That should be satisfied by the browserify operation that will omit node-fetch since its value is false in package.json's browser property:

"browser": { "node-fetch": false, "abort-controller": false, "./lib/package_version": "./lib/package_version_browser" },

I checked airtable.browser.js and verified that node-fetch has an empty module entry 👍

SomeHats · 2020-08-17T14:40:28Z

src/query.ts

+    readonly firstPage: RecordCollectionRequestMethod;
+    readonly eachPage: RecordPageIteratationMethod;
+    readonly all: RecordCollectionRequestMethod;


out of curiosity, why this approach vs. standard typescript method overloading?

oh i guess to support callbackToPromise

Yeah. I couldn't think of a more standard typescript approach without removing the use of callbackToPromise.

SomeHats · 2020-08-17T14:45:27Z

src/record.ts

+}
+
+function patchUpdate(this: Record, cellValuesByName, opts, done?: RecordCallback) {
+    const that = this;


because these now use arrow functions, i believe that this 'that this' (????? these words are hard to talk about lol) is no longer needed(ditto for elsewhere in this file/project)

SomeHats · 2020-08-17T14:49:32Z

src/table.ts

+        done: RecordCollectionCallback
+    ): void;
+    _createRecords(recordsData, optionalParameters, done?) {
+        const that = this;


ditto re: that/this

SomeHats · 2020-08-17T14:55:37Z

src/airtable.ts

+    readonly _endpointUrl: string;
+    readonly _apiVersion: string;
+    readonly _apiVersionMajor: string;
+    readonly _noRetryIfRateLimited: boolean;


we should mark these as private, but our style guide still encourages prefixing private methods with _

SomeHats

thanks for the updates! this looks all-good to me (pending that package.json update for the type definitions)

mzgoddard · 2020-08-24T15:36:53Z

Add "types" field to package json

SomeHats

Hey folks! Finally got around to running the integration tests for this repo - sorry for the delay! There are a few issues with the types, but i didn't see any actual functional errors. I left an inline comment on the main type issue (a bad inferred return type) but could we potentially go through this diff and make sure that every method has its arguments and return value correctly typed? could also introduce some lint rules enforcing this as part of this diff

SomeHats · 2020-08-31T18:05:05Z

src/base.ts

+        return this._id;
+    }
+
+    static createFunctor(airtable: Airtable, baseId: string) {


Can we define a return type for this that includes all the correct methods and properties? things like new Airtable().base(someId).table(someTable) are errors otherwise.

@SomeHats I'm working on this now, and something I ran into that I can see only one way around. When I define a type that includes the methods that are added below in the forEach(['table', 'makeRequest', 'runAction', 'getId'], ... operation, they are not recognized. When I define them as standalone assignments (statically analyzable), then it all works out. The only issue is that it's not very nice to look at.

Here's the type I defined:

type AirtableBase = { (tableName: any): Table; _base: Base; getId(): string; makeRequest(options: BaseRequestOptions): Promise<Record<string, any>>; table(tableName: string): Table; tables: any; };

what if you turn line 218 into return baseFn as AirtableBase? that should let us keep the correct types for the return value and not have to worry too much about how it's constructed.

re: that AirtableBase type, a few questions:

can we replace all the anys with more specific types?

can we get rid of _base from the public type definition? if no that's fine

any idea what's up with tables? i don't see it defined on Base is it just undefined? should we remove it?

what if you turn line 218 into return baseFn as AirtableBase? that should let us keep the correct types for the return value and not have to worry too much about how it's constructed.

Smart—that does the trick for the existing forEach(['table', 'makeRequest', 'runAction', 'getId'], ... operation.

re: that AirtableBase type, a few questions:

can we replace all the anys with more specific types?

Sure can!

can we get rid of _base from the public type definition? if no that's fine

Unfortunately, when I remove that property it produces errors.

any idea what's up with tables? i don't see it defined on Base is it just undefined? should we remove it?

Digging around and I learned that there is no other reference to this property anywhere else in the codebase—I'm going to nuke it.

rwaldron · 2020-09-01T18:40:59Z

src/base.ts

+    getId(): string;
+    makeRequest(options: BaseRequestOptions): Promise<BaseResponse>;
+    table(tableName: string): Table;
+};


@SomeHats PTAL

rwaldron · 2020-09-01T18:41:03Z

src/base.ts

@@ -214,8 +222,7 @@ class Base {
            baseFn[baseMethod] = base[baseMethod].bind(base);
        });
        baseFn._base = base;
-        baseFn.tables = base['tables'];
-        return baseFn;
+        return baseFn as AirtableBase;


@SomeHats PTAL

- Use grunt-ts with recommended minimal config: https://github.com/TypeStrong/grunt-ts#getting-started - Switch all filetypes to `ts` - Ignore all warnings and imply all types

- eslint only looks at ts files by default. Add ts files to files reviewed. - Add recommened tslint pluggin - Switch all linting errors to warnings for now. Will go back and switch them to warnings at a future commit.

- Change the path of the required files in the tests to test the build files - Change the test runner to compile the typescript prior to running the tests - Change the test runner to only run the tests once (coverage also runs the tests) - Pass --no-cache to coverage to ensure it works with typescript: kulshekhar/ts-jest#211 - Stop using `grunt-ts` because it doesn't actually just pass the config through tsc. This caused problems when the new resolveJsonModule config didn't get passed through. Instead just use `tsc` to compile directly. - Modify the `tsconfig` to ensure that json files are included in the compiled package. - One line of code is no longer coverage for mysterious reasons, punt on figuring out why and just ignore it so they test continue to pass.

Source the files from the compiled js instead of the source typescript. At this point grunt doesn't seem to be adding much value because I ended up adding the task in package.json directly but removing it is out of scope.

- This is a more traditional location and it fixes problems with the location of package.json

- type param validators and by extensions the params to Query - type bound methods with callbackToPromise with overloaded interfaces

/lib/ is tsc's configured output now. /lib/ is not longer committed to git.

- do not lint build directory lib/* - output source maps from typescript files for coverage testing - fix a istanbul ignore in fetch.ts

rwaldron · 2020-09-01T18:48:18Z

I've just rebased this with master, so it will apply cleanly.

rwaldron · 2020-09-03T15:08:42Z

src/base.ts

-    qs?: any;
-    headers?: any;
-    body?: any;
+    qs?: Record<string, any>;


@SomeHats in Airtable/Base: Pass custom headers to requests made through the client, I've introduced the ObjectMap we discussed in Slack. Once this PR lands and then that PR lands, we can come back use ObjectMap here as well.

sounds good to me!

SomeHats

great stuff! integration tests are all passing, i'll look at getting this landed soon :D

richsinn requested review from SomeHats and emmettnicholas July 30, 2020 15:43

mzgoddard force-pushed the mzgoddard-typescriptIt branch 3 times, most recently from 6d753b0 to 97004bb Compare August 5, 2020 16:29

rwaldron reviewed Aug 6, 2020

View reviewed changes

mzgoddard force-pushed the mzgoddard-typescriptIt branch from 7a18194 to a4626f6 Compare August 10, 2020 19:15

SomeHats suggested changes Aug 17, 2020

View reviewed changes

richsinn mentioned this pull request Aug 18, 2020

Upgrading to Typescript and the Underlying Core #200

Closed

SomeHats approved these changes Aug 21, 2020

View reviewed changes

mzgoddard mentioned this pull request Aug 29, 2020

typescript follow up bocoup/airtable.js#2

Draft

SomeHats suggested changes Aug 31, 2020

View reviewed changes

rwaldron reviewed Sep 1, 2020

View reviewed changes

rmeritz and others added 14 commits September 1, 2020 14:47

Switch build to typescript

be39d3b

- Use grunt-ts with recommended minimal config: https://github.com/TypeStrong/grunt-ts#getting-started - Switch all filetypes to `ts` - Ignore all warnings and imply all types

Lint ts files with eslint

35619d4

- eslint only looks at ts files by default. Add ts files to files reviewed. - Add recommened tslint pluggin - Switch all linting errors to warnings for now. Will go back and switch them to warnings at a future commit.

Fix browerify to work with typescript

d656e0d

Source the files from the compiled js instead of the source typescript. At this point grunt doesn't seem to be adding much value because I ended up adding the task in package.json directly but removing it is out of scope.

Move build from build/dist to lib

45df59d

- This is a more traditional location and it fixes problems with the location of package.json

typescript: add types to public interfaces

c804f99

- type param validators and by extensions the params to Query - type bound methods with callbackToPromise with overloaded interfaces

typescript: add @types/lodash as dev dependency

c70fbfc

typescript: add /lib/ to prettierignore

dbab7e9

/lib/ is tsc's configured output now. /lib/ is not longer committed to git.

typescript: make grunt-ts and typescript lint parser dev deps

35e6ceb

typescript: compile before publish or after local install

58217c9

typescript: do not lint lib/* and do coverage with sourcemaps

001ee7a

- do not lint build directory lib/* - output source maps from typescript files for coverage testing - fix a istanbul ignore in fetch.ts

typescript: lint: remove accidental space from run_action

4a671bb

typescript: remove grunt-ts dependency

c0aa0db

typescript: add types to AirtableError members

b6516fb

mzgoddard and others added 4 commits September 1, 2020 14:47

typescript: rename that to this

d74c0ea

typescript: output .d.ts files

42443b9

typescript: add types field to package json

ce60e6d

Add AirtableBase return type

c7b74ca

rwaldron force-pushed the mzgoddard-typescriptIt branch from cb8c3d4 to c7b74ca Compare September 1, 2020 18:48

This was referenced Sep 1, 2020

Restrict typescript lib #211

Merged

Remove unnecessary iiafes #212

Merged

Pass custom headers to requests made through the client #213

Merged

rwaldron reviewed Sep 3, 2020

View reviewed changes

SomeHats approved these changes Sep 3, 2020

View reviewed changes

SomeHats merged commit dc07837 into Airtable:master Sep 4, 2020

SomeHats mentioned this pull request Sep 8, 2020

fix package.json scripts & rebuild #215

Merged

jugglinmike mentioned this pull request Sep 23, 2020

Increase dependency granularity to avoid superfluous warnings and reduce package size #206

Closed

		@@ -1,31 +1,23 @@
		'use strict';
		import exponentialBackoffWithJitter from './exponential_backoff_with_jitter';

		@@ -0,0 +1,24 @@
		import includes from 'lodash/includes';
		import isArray from 'lodash/isArray';


		options = options \|\| {};

		const method = get(options, 'method', 'GET').toUpperCase();

Translate source code to typescript #197

Translate source code to typescript #197

Conversation

mzgoddard commented Jul 28, 2020

rwaldron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzgoddard Aug 10, 2020 • 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

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

mzgoddard commented Aug 10, 2020

SomeHats 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SomeHats left a comment

Choose a reason for hiding this comment

mzgoddard commented Aug 24, 2020

SomeHats 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

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

rwaldron commented Sep 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SomeHats left a comment

Choose a reason for hiding this comment

mzgoddard Aug 10, 2020 •

edited

Loading