Add initial Typescript support #102

aalmazan · 2022-04-01T00:30:25Z

Description of the change

Adds tsconfig.json and typescript along with some updates due to tsc compiler errors.

Type of change

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Related issues

Fixes

Checklists

Development

The code changed/added as part of this pull request has been covered with tests
All tests related to the changed code pass in development
No previous tests unrelated to the changed code fail in development

Code review

This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached.
At least one reviewer has been requested
Changes have been reviewed by at least one other engineer
The relevant project board has been selected in Projects to auto-link to this pull request

aalmazan · 2022-04-01T00:31:01Z

src/index.js

-
-  static _prepareGraphQLBase64 (data, options = {}) {
-    const { filename, mimetype } = options
-    if (!filename) {
-      throw new Error('options.filename must be provided for Base64 upload')
-    }
-    if (!mimetype) {
-      throw new Error('options.mimetype must be provided for Base64 upload')
-    }
-
-    if (options.bufferize) {
-      const buffer = Buffer.from(data, 'base64')
-      return this._prepareGraphQLBuffer(buffer, options)
-    }
-
-    return {
-      data,
-      filename,
-      mimetype,
-    }
-  }


Got an error inside this method but it seems like we don't use it anymore?

newhouse · 2022-04-01T22:27:05Z

I'm not 100% following what the goal is here?

Is it to write the source code in Typescript and then boil it down to JS? That's what I would think the goal is, right? If that's the case, then there are a few things to fix here:

The tsconfig should not have emitDeclarationOnly: true...we want the compiler to output JS files from the TS source files.
The tsconfig should carefully choose the target and lib values to match with the version of Node that we want the package to support. It's basically like telling it how to babelize things.
The /dist folder should not be checked into github, and should be .gitignore'd. The output data should be an artifact of the TS compiling command. However, the /dist folder will be included in NPM publish, and will end up on Users' machines when they install this package.
The source files should probably be renamed to *.ts.

I can walk you through all this and whatever else I know about Typescript and Node / NPM packages...ping me!

aalmazan · 2022-04-04T02:52:43Z

I'm not 100% following what the goal is here?

This PR isn't for converting the code base to have typescript support, but to have initial typescript *.d.ts files (issue linked above your comment).

The tsconfig should carefully choose the target and lib values to match with the version of Node that we want the package to support. It's basically like telling it how to babelize things.

Yeah probably needed to pay attention to that part of the config. Switching to:

    "target": "es2021",
    "module": "commonjs",

The /dist folder should not be checked into github, and should be .gitignore'd. The output data should be an artifact of the TS compiling command. However, the /dist folder will be included in NPM publish, and will end up on Users' machines when they install this package.

Yes this is on purpose. From the typescript docs, ~~the generated .d.ts file path resolution defaults will eventually check dist/. IMO I'd prefer that over having *.d.ts files beside every .ts|js file.~~

EDIT: Changed it to a more obvious types/ dir since I glossed over the part about defining it in package.json. Updated.

benogle

Looks good to me! The last thing would be to make sure it can be consumed by a toy typescript project

benogle · 2022-04-04T17:32:57Z

src/index.js

@@ -1,5 +1,6 @@
 const fs = require('fs')

+const { ReadStream } = require('fs')


was this being used before without import?

Oops. That snuck in while I was updating something. Removing.

benogle · 2022-04-04T17:35:32Z

types/src/index.d.ts

+    rateLimiterPromiseResolver: (value: any) => void;
+    _setRateLimiter({ tokens, intervalMs }: {
+        tokens: any;
+        intervalMs: any;


Any easy way to inject types for simple things like this? i.e. this should be an int

Yeah looks like it should generate properly with JSDoc comments. Will add that.

benogle · 2022-04-04T17:35:45Z

types/src/index.d.ts

+        responseQuery: any;
+        mutation: any;


strings here

newhouse · 2022-04-04T18:50:26Z

package.json

@@ -3,6 +3,7 @@
  "version": "2.9.4",
  "description": "Anvil API Client",
  "main": "src/index.js",
+  "types": "./types/index.d.ts",


newhouse · 2022-04-04T18:59:22Z

seems like a fair amount of JSDoc'ing could be added to make this really worth it?

aalmazan added 4 commits March 31, 2022 17:28

Add typescript and node types

c27b22c

Add typescript types

8e25ca4

Add generated .d.ts files

d28d396

Fixes for tsc checks

25fe07c

aalmazan requested review from benogle and newhouse April 1, 2022 00:30

aalmazan commented Apr 1, 2022

View reviewed changes

Update tests

b31f3e1

aalmazan added 2 commits April 3, 2022 19:53

Better target/module

e123efb

Move dist -> types, "types" in package.json

627b5c1

benogle approved these changes Apr 4, 2022

View reviewed changes

newhouse approved these changes Apr 4, 2022

View reviewed changes

aalmazan added 2 commits April 4, 2022 13:05

Add JSDoc comments for some func params

4c28568

Variables optional

a678fa2

aalmazan merged commit 1c4d07d into master Apr 5, 2022

aalmazan deleted the aa/initial-ts-support branch April 5, 2022 00:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial Typescript support #102

Add initial Typescript support #102

aalmazan commented Apr 1, 2022 •

edited

Loading

aalmazan Apr 1, 2022

newhouse commented Apr 1, 2022

aalmazan commented Apr 4, 2022 •

edited

Loading

benogle left a comment

benogle Apr 4, 2022

aalmazan Apr 4, 2022

benogle Apr 4, 2022

aalmazan Apr 4, 2022

benogle Apr 4, 2022

newhouse Apr 4, 2022

newhouse commented Apr 4, 2022

		@@ -1,5 +1,6 @@
		const fs = require('fs')

		const { ReadStream } = require('fs')

Add initial Typescript support #102

Add initial Typescript support #102

Conversation

aalmazan commented Apr 1, 2022 • edited Loading

Description of the change

Type of change

Related issues

Checklists

Development

Code review

aalmazan Apr 1, 2022

Choose a reason for hiding this comment

newhouse commented Apr 1, 2022

aalmazan commented Apr 4, 2022 • edited Loading

benogle left a comment

Choose a reason for hiding this comment

benogle Apr 4, 2022

Choose a reason for hiding this comment

aalmazan Apr 4, 2022

Choose a reason for hiding this comment

benogle Apr 4, 2022

Choose a reason for hiding this comment

aalmazan Apr 4, 2022

Choose a reason for hiding this comment

benogle Apr 4, 2022

Choose a reason for hiding this comment

newhouse Apr 4, 2022

Choose a reason for hiding this comment

newhouse commented Apr 4, 2022

aalmazan commented Apr 1, 2022 •

edited

Loading

aalmazan commented Apr 4, 2022 •

edited

Loading