Skip to content

Commit

Permalink
Remove Typebox and AVJ to work better with Edge runtimes (#249)
Browse files Browse the repository at this point in the history
## Problem

There have been a litany of issues related to the incompatibility of our
current Typescript client and Edge runtimes (e.g. issue
[164](#164),
issue
[212](#212),
[this](https://community.pinecone.io/t/error-using-node-js-sdk-on-edge/6337/4)
community forum post).

The long and the short of it is that we use two libraries:
sinclair/typebox and ajv that to do not work in Edge runtimes. The
result is that users need to downgrade their client version in order to
use our client. This has the major disadvantage of meaning that these
users don't get access to our client's newest features (e.g. inference).

Asana ticket:
https://app.asana.com/0/1203260648987893/1208002049018710/f

## Solution

Remove typebox and ajv!

## Approach
- Previously we had custom typebox schema objects declared in `types.ts`
files under `control` and `data`. This PR removes those typebox schemas.
We used those typebox schemas to do things like ensure the correct
properties were sent with requests like `Create Index`, which needs a
particular `Spec` obj with particular fields, etc. This PR maintains
that behavior, but through custom validation functions embedded within
the client's native objects (e.g. `QueryCommand`, `createIndex`, etc.)
themselves.

- Importantly, these internal changes **do not result** in any
downstream changes the user should be made aware of -- all exported
Pinecone `types` remain untouched in files like `index.ts` under `data`
and `control`.

- This PR also introduces new unit and integration tests to ensure the
above-mentioned functionality. It also removes unit and integration
tests that simply test whether a particular field's value is of a
particular data type.
- I chose to remove these tests since they're redundant in Typescript.
The thinking here is that: if we're going to write a Typescript client
and glean the benefits of using Typescript (rather than Javascript), we
need to rely on Typescript's typing power. Otherwise, we should just
write a Javascript client. I know this is unideal for non-Typescript
users of our client, but the backend API will throw errors at runtime if
non-TS users pass in the incorrect datatypes, and this is enough for
now.

- Finally, this PR introduces checks for unknown field properties/names.
If a user passes in `diemnsion` instead of `dimension` when creating an
index, an error will be thrown that tells the user 1) what field was
identified as unknown and 2) a list of all the acceptable fields for
that object.

## Type of Change

- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to not work as expected)
- [ ] This change requires a documentation update
- [ ] Infrastructure change (CI configs, etc)
- [ ] Non-code change (docs, etc)
- [ ] None of the above: (explain here)

## Test Plan

CI passes.
  • Loading branch information
aulorbe authored Aug 19, 2024
1 parent d38a6ad commit 8bffd77
Show file tree
Hide file tree
Showing 44 changed files with 1,193 additions and 1,681 deletions.
5 changes: 3 additions & 2 deletions .github/workflows/testing.yml
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,8 @@ jobs:
strategy:
fail-fast: false
matrix:
tsVersion: [
tsVersion:
[
'~4.1.0',
'~4.2.0',
'~4.3.0',
Expand All @@ -111,7 +112,7 @@ jobs:
'~5.0.0',
'~5.1.0',
'~5.2.0',
# 'latest', - typebox breaks with latest TypeScript, re-enable later
'latest',
]
steps:
- name: Checkout
Expand Down
35 changes: 3 additions & 32 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 2 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
"contributors": [
"Jen Hamon (https://github.com/jhamon)",
"Roie Schwaber-Cohen (https://github.com/rschwabco)",
"Austin DeNoble (https://github.com/austin-denoble)"
"Austin DeNoble (https://github.com/austin-denoble)",
"Audrey Lorberfeld (https://github.com/aulorbe)"
],
"license": "Apache-2.0",
"scripts": {
Expand Down Expand Up @@ -51,8 +52,6 @@
"/dist"
],
"dependencies": {
"@sinclair/typebox": "^0.29.0",
"ajv": "^8.12.0",
"cross-fetch": "^3.1.5",
"encoding": "^0.1.13"
}
Expand Down
9 changes: 6 additions & 3 deletions src/__tests__/pinecone.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,8 @@ describe('Pinecone', () => {
expect(() => {
new Pinecone({} as PineconeConfiguration);
}).toThrow(
'The client configuration must have required property: apiKey.'
'The client configuration must have required property: apiKey. You can find the configuration values for' +
' your project in the Pinecone developer console at https://app.pinecone.io.'
);
});

Expand All @@ -81,7 +82,8 @@ describe('Pinecone', () => {
} as PineconeConfiguration;
new Pinecone(config);
}).toThrow(
"The client configuration had validation errors: property 'apiKey' must not be blank."
'The client configuration must have required property: apiKey. You can find the configuration values for' +
' your project in the Pinecone developer console at https://app.pinecone.io.'
);
});
});
Expand All @@ -94,7 +96,8 @@ describe('Pinecone', () => {
unknownProp: 'banana',
} as PineconeConfiguration);
}).toThrow(
'The client configuration had validation errors: argument must NOT have additional properties.'
'Object contained invalid properties: unknownProp. Valid properties include apiKey, controllerHostUrl,' +
' fetchApi, additionalHeaders, sourceTag.'
);
});
});
Expand Down
Loading

0 comments on commit 8bffd77

Please sign in to comment.