Configure airtable.js work as a typescript library #1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- 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.
mzgoddard
suggested changes
Jul 1, 2020
| "format": "prettier --write '**/*.js'", | ||
| "test": "npm run coverage && npm run test-unit", | ||
| "test-unit": "jest --env node" | ||
| "test": "npm run coverage", |
There was a problem hiding this comment.
Should this still be npm run coverage && npm run test-unit
There was a problem hiding this comment.
The tests used to be running twice. Now they are only runing once which I think is more responable.
| }, | ||
| "dependencies": { | ||
| "@types/node": "^14.0.14", | ||
| "@typescript-eslint/parser": "^3.4.0", |
There was a problem hiding this comment.
I don't think we need ts eslint parser in dependencies.
There was a problem hiding this comment.
We do. Otherwise I get an error:
Error: Failed to load parser '@typescript-eslint/parser' declared in '.eslintrc.json » plugin:@typescript-eslint/recommended » ./configs/base$
: Cannot find module '@typescript-eslint/parser'
Require stack:
- /home/rebecca/airtable/airtable.js/node_modules/@typescript-eslint/eslint-plugin/dist/configs/base.js
at Function.Module._resolveFilename (internal/modules/cjs/loader.js:957:15)
at Function.resolve (internal/modules/cjs/helpers.js:83:19)
at Object.resolve (/home/rebecca/airtable/airtable.js/node_modules/eslint/lib/shared/relative-module-resolver.js:44:50)
at ConfigArrayFactory._loadParser (/home/rebecca/airtable/airtable.js/node_modules/eslint/lib/cli-engine/config-array-factory.js:870:45)
at ConfigArrayFactory._normalizeObjectConfigDataBody (/home/rebecca/airtable/airtable.js/node_modules/eslint/lib/cli-engine/config-array-f
actory.js:665:32)
at _normalizeObjectConfigDataBody.next (<anonymous>)
at ConfigArrayFactory._normalizeObjectConfigData (/home/rebecca/airtable/airtable.js/node_modules/eslint/lib/cli-engine/config-array-facto
ry.js:596:20)
at _normalizeObjectConfigData.next (<anonymous>)
at ConfigArrayFactory._normalizeObjectConfigDataBody (/home/rebecca/airtable/airtable.js/node_modules/eslint/lib/cli-engine/config-array-f
actory.js:660:25)
at _normalizeObjectConfigDataBody.next (<anonymous>)
tsconfig.json
Outdated
| "resolveJsonModule": true, | ||
| "esModuleInterop": true, | ||
| "sourceMap": true, | ||
| "outDir": "build/dist", |
There was a problem hiding this comment.
A common typescript idiom is to build to the lib directory. We can rename lib to src, outDir to lib, and git ignore the lib folder after the rename.
There was a problem hiding this comment.
I believe but don't fully recall that this is a longer running transpiled package idiom like with coffeescript/etc.
There was a problem hiding this comment.
You suggested moving the build ts from build/dist to lib and the source to src. If I do that I end up getting the built code located at lib/src which I think looks really weird and confusing. I cannot just remove the src dir in the built package because we also need to pull in package.json (for the version #) into the built package. Should I still try and follow this idiom?
~/airtable/airtable.js% ls lib/**/*(.)
lib/package.json lib/src/deprecate.js.map lib/src/package_version.js.map
lib/src/abort-controller.d.ts lib/src/exponential_backoff_with_jitter.js lib/src/promise.js
lib/src/abort-controller.js lib/src/exponential_backoff_with_jitter.js.map lib/src/promise.js.map
lib/src/abort-controller.js.map lib/src/fetch.js lib/src/query.js
lib/src/airtable.d.ts lib/src/fetch.js.map lib/src/query.js.map
lib/src/airtable_error.d.ts lib/src/has.js lib/src/record.js
lib/src/airtable_error.js lib/src/has.js.map lib/src/record.js.map
lib/src/airtable_error.js.map lib/src/http_headers.js lib/src/run_action.js
lib/src/airtable.js lib/src/http_headers.js.map lib/src/run_action.js.map
lib/src/airtable.js.map lib/src/internal_config.json lib/src/table.js
lib/src/base.js lib/src/object_to_query_param_string.js lib/src/table.js.map
lib/src/base.js.map lib/src/object_to_query_param_string.js.map lib/src/typecheck.js
lib/src/callback_to_promise.js lib/src/package_version_browser.js lib/src/typecheck.js.map
lib/src/callback_to_promise.js.map lib/src/package_version_browser.js.map
lib/src/deprecate.js lib/src/package_version.js
There was a problem hiding this comment.
I fixed this issue by removing package.json from the tsconfig.
| @@ -4,7 +4,8 @@ | |||
| "node": true | |||
| }, | |||
| "extends": [ | |||
| "eslint:recommended" | |||
| "eslint:recommended", | |||
| "plugin:@typescript-eslint/recommended" | |||
| "sourceMap": true, | ||
| "outDir": "build/dist", | ||
| "noImplicitAny": false, | ||
| "strict": false |
There was a problem hiding this comment.
Add "declaration": true".
Optionally we can add "declarationMap": true.
There was a problem hiding this comment.
I don't think we can add this now to "Generates corresponding d.ts files."
When I add it tsc just crashes with the following error:
~/airtable/airtable.js% node_modules/typescript/bin/tsc
/home/rebecca/airtable/airtable.js/node_modules/typescript/lib/tsc.js:82902
throw e;
^
TypeError: Cannot read property 'parent' of undefined
at transformTopLevelDeclaration (/home/rebecca/airtable/airtable.js/node_modules/typescript/lib/tsc.js:76323:55)
at visitDeclarationStatements (/home/rebecca/airtable/airtable.js/node_modules/typescript/lib/tsc.js:76270:26)
at Object.visitNodes (/home/rebecca/airtable/airtable.js/node_modules/typescript/lib/tsc.js:63024:48)
at transformRoot (/home/rebecca/airtable/airtable.js/node_modules/typescript/lib/tsc.js:75725:37)
at transformation (/home/rebecca/airtable/airtable.js/node_modules/typescript/lib/tsc.js:76815:24)
at transformRoot (/home/rebecca/airtable/airtable.js/node_modules/typescript/lib/tsc.js:76833:82)
at Object.map (/home/rebecca/airtable/airtable.js/node_modules/typescript/lib/tsc.js:335:29)
at Object.transformNodes (/home/rebecca/airtable/airtable.js/node_modules/typescript/lib/tsc.js:76820:30)
at emitDeclarationFileOrBundle (/home/rebecca/airtable/airtable.js/node_modules/typescript/lib/tsc.js:77362:43)
at emitSourceFileOrBundle (/home/rebecca/airtable/airtable.js/node_modules/typescript/lib/tsc.js:77278:13)
I'm not confident the source of the error, but I think it might make more sense to try and add this config when the files have valid types.
- This is a more traditional location and it fixes problems with the location of package.json
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The PR is not actual for merging it is just to facilitate an internal review. I cannot open up a real PR on top of the real airtable.js until Airtable#191 is merged.
jsfiles totsfiles.compiletasks that compiles thetstojsusingtsc. I had originally been using thegrunt-tswrapper, but it caused lots of problems (It didn't just pass through the config totsceven though the docs said it did) so I just switched to usingtscdirectly.tsconfig.jsto control specifics of the compilation. Currently, errors are emitted from the complication but it goes on anyway. Once the library is updated to truly be written in typescript those configuration options can be changed.tsplugin toeslintwith the recommended rules. In order to stop it from erroring switch all the new errors to warnings. Those warnings can and should be removed once the typescript project is complete.jsfiles instead of the source files directly. I originally tried usingts-jestbut I ran into problems gettingts-jestto compile without emitting errors and crashing so I decided to just reuse the existing compilation task for testing.testcommand to only run the tests once and work with typescript.This can be built upon to do the actual work for converting the
jsto valid usefults.