Resolve typescript compilation problems #114

jhamon · 2023-09-14T03:27:01Z

Problem

Several users have reported issues with typescript compilation

This PR addresses:

Solution

Clean up unused dependencies to resolve web-related type errors

I started by deleting dependencies and running tests to see what dependencies were no longer in use. I was able to delete these:

@types/web
@jest/globals
unique-names-generator

Removing @types/web seems to have resolved the majority of Typescript issues related to conflicts with web types in lib.dom.d.ts. Since I was already in there mucking with dependencies, I went ahead and shuffled a few things between dependencies/devDependencies just to align with my current understanding of things.

TS1005 (Typescript versions <=4.4)

People on older typescript versions report seeing many errors like this:

Error: ../pinecone-ts-client/dist/control/index.d.ts(1,31): error TS1005: ',' expected.

These errors were caused by using import statements that combined type and value imports onto one line. This syntax was evidently not supported in versions of typescript prior to 4.4.

Resolving these issues involved refactoring about 30 import statements along these lines:

-import { IndexNameSchema, type IndexName } from './types';
+import { IndexNameSchema } from './types';
+import type { IndexName } from './types';

Typebox errors (Typescript >= 5.1)

The @sinclair/typebox dependency is causing issues with very recent versions of typescript.

../node_modules/@sinclair/typebox/typebox.d.ts:179:167 - error TS2589: Type instantiation is excessively deep and possibly infinite.

179 ] extends [TArray, TNumber] ? AssertType<T['items']> : K extends TTemplateLiteral ? TIndexReduce<T, TTemplateLiteralKeyRest<K>> : K extends TUnion<TLiteral<Key>[]> ? TIndexReduce<T, TUnionLiteralKeyRest<K>> : K extends TLiteral<Key> ? TIndexReduce<T, [K['const']]> : TNever;

New CI job: `typescript-compilation-tests`

To verify this compilation issue doesn't come up again, I added a new github actions job along with a version matrix. It tests every major version of typescript back to version 4.1 (released fall 2020) along with a version of pinecone, then checks whether it can successfully tsc. I ran these with the v1.0.0 pinecone release to see the jobs fail as reported, then updated it to install the latest code in this branch and saw them pass. This will be part of CI for every PR from now on.

Type of Change

Bug fix (non-breaking change which fixes an issue)
This change requires a documentation update
Infrastructure change (CI configs, etc)

Test Plan

Should see new compilation tests passing in CI. There are now 13 different checks running in CI to exercise compilation with major versions of TypeScript.

Follow-up

Probably we should add a lint rule to prevent us adding back any mixed type/value imports.

jhamon · 2023-09-14T08:42:37Z

package.json

    "@edge-runtime/types": "^2.2.3",
-    "@sinclair/typebox": "^0.28.15",
-    "@types/web": "^0.0.99",
+    "@sinclair/typebox": "^0.29.0",


Typebox got a small bump to resolve an error on TS >5.1. I didn't bump all the way to latest because that caused problems with some major releases of TS 4.x.

jhamon · 2023-09-14T08:43:38Z

src/control/index.ts

-  type IndexList,
-  type PartialIndexDescription,
-} from './listIndexes';
+export { configureIndex } from './configureIndex';


These formatting changes don't read nearly as nice, but are necessary for compatibility with older versions of typescript.

jhamon · 2023-09-14T08:45:16Z

src/utils/os.ts

@@ -2,6 +2,8 @@ import * as os from 'os';

 export const getOSInfo = () => {
  // If available, capture information about the OS
+
+  // @ts-ignore


I'm actually deleting this file in a different PR, but TS 4.1 was upset that I was checking all these things in the if-statement that its knowledge of types told it should always be defined. Regardless of what the type says, i think it's prudent to be cautious regarding things you expect from the environment.

jhamon · 2023-09-14T08:46:23Z

src/v0/index.ts

-    newResult.set(result);
-    newResult.set(value, result.length);
-    result = newResult;
+    if (value) {


Older TS compilers were erroring on the value.length reference below because the type says value could be undefined. I don't think it's a realistic case you would encounter in runtime, though due to the break on done condition above.

austin-denoble

Great work getting all these issues resolved so quickly. Overall this looks good, and thanks for walking through the thinking around the approach. I think explicitly covering variations in TS versions will be really helpful for us moving forward, especially given a lot of the feedback coming in post V1. 🚢

austin-denoble · 2023-09-14T16:50:11Z

.gitignore

question: I know this was in here before, but Is scrap needed? I can't see how or where it'd be generated.

austin-denoble · 2023-09-14T16:51:55Z

package.json

  },
  "types": "dist/index.d.ts",
  "files": [
    "/dist"
  ],
  "dependencies": {
-    "@edge-runtime/jest-environment": "^2.3.3",


I was looking at package.json earlier and was questioning whether this should be in dependencies or devDependencies, nice.

austin-denoble · 2023-09-14T16:58:03Z

.github/workflows/pr.yml

+          cat package.json
+          cat package-lock.json


nit: I'm assuming we want to keep these lines for debugging / visibility in the CI run output, but just in case they were leftover from debugging, etc.

Yeah, my thinking was that it would be useful to be able to see what the commands actually installed. To speed debugging also in case of future failiures.

austin-denoble · 2023-09-14T16:58:32Z

.github/workflows/pr.yml

+    runs-on: ubuntu-latest
+    strategy:
+      fail-fast: false
+      matrix:


praise: It's a lot of overhead to validate against so many TS versions, but I feel like you've done a really elegant job here. Nice work.

jhamon changed the title ~~Remove @types/web which seems unused~~ Cleanup package.json dependencies Sep 14, 2023

jhamon changed the title ~~Cleanup package.json dependencies~~ Resolve typescript compilation problems Sep 14, 2023

jhamon requested review from austin-denoble and rohanshah18 September 14, 2023 07:45

jhamon marked this pull request as ready for review September 14, 2023 07:45

jhamon commented Sep 14, 2023

View reviewed changes

austin-denoble approved these changes Sep 14, 2023

View reviewed changes

jhamon added 2 commits September 14, 2023 15:38

Squash and rebase

b7369c1

Review feedback

3657da7

jhamon force-pushed the jhamon/typescript-compilation-errors branch from 2e93b54 to 3657da7 Compare September 14, 2023 19:41

jhamon merged commit 4ef21d3 into main Sep 14, 2023
17 checks passed

jhamon deleted the jhamon/typescript-compilation-errors branch September 14, 2023 19:42

jhamon mentioned this pull request Sep 15, 2023

[Bug] v1.0.0 transpile errors #102

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve typescript compilation problems #114

Resolve typescript compilation problems #114

jhamon commented Sep 14, 2023 •

edited

Loading

jhamon Sep 14, 2023

jhamon Sep 14, 2023

jhamon Sep 14, 2023

jhamon Sep 14, 2023

austin-denoble left a comment

austin-denoble Sep 14, 2023

austin-denoble Sep 14, 2023

austin-denoble Sep 14, 2023

jhamon Sep 14, 2023

austin-denoble Sep 14, 2023

Resolve typescript compilation problems #114

Resolve typescript compilation problems #114

Conversation

jhamon commented Sep 14, 2023 • edited Loading

Problem

Solution

Clean up unused dependencies to resolve web-related type errors

TS1005 (Typescript versions <=4.4)

Typebox errors (Typescript >= 5.1)

New CI job: typescript-compilation-tests

Type of Change

Test Plan

Follow-up

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

austin-denoble 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

jhamon commented Sep 14, 2023 •

edited

Loading

New CI job: `typescript-compilation-tests`