xataio / client-ts Goto Github PK

So while creating example apps, I have a file ./generated/XataClient.ts that contains generated code. From here, I'm not sure what to do with this class in the context of a Next.js app. Particularly, do I instantiate it every time inside getServerSideProps and co? Or do I instantiate it in module scope in ./util/client.ts and import it where I need it?

I think module scope might make sense because otherwise, we'd be creating a new client on every invocation of getServerSideProps that is identical in behavior to the previous invocation, consuming unnecessary compute which is billed on most platforms, including Vercel. Module scope is preserved between serverless function (like getServerSideProps) invocations as long as the function is warm, so this will probably be the more efficient route.

So now–if it's in module scope–would it be a convenience/service to the user to export an instance of this class directly from ./generated/XataClient.ts? That would save them the manual work of creating another file with an export. I guess the only limitation here is we'd assume branch: "main" as default, but seems like we really don't want to do that because it's unsafe.

Well in this case, I guess I have my answer. 😬 Thanks for the rubber ducky session. Closing...

Even though I'm still a couple of weeks from onboarding, I had some curiosity about the state of the TS client, and I took a look to the code. I've noticed that nested property access from string was not possible yet (even if the API supports it).

When string literal types became a thing on TS I experimented a bit with them and I think they can be useful here. I've quickly adapted a small playground I had from a few months ago to something similar to what the API supports for selects and filters. Just leaving it here so you can take a look.

type StringKeys<O> = Extract<keyof O, string>;
type Values<O> = O[keyof O];

export type ObjectPaths<O> = O extends Array<unknown>
  ? never // Not sure about how the API will handle arrays of elements (multiple field type seems to be string only and it's not clear if it will be generic)
  : O extends Record<string, any>
  ?
      | "*"
      | Values<{
          [K in StringKeys<O>]: O[K] extends Record<string, any>
            ? `${K}.${ObjectPaths<O[K]>}`
            : K;
        }>
  : "";

export type ObjectProperty<O, P extends ObjectPaths<O>> = P extends "*"
  ? Values<O>
  : P extends keyof O
  ? O[P]
  : P extends `${infer K}.${infer V}`
  ? K extends keyof O
    ? V extends ObjectPaths<O[K]>
      ? ObjectProperty<O[K], V>
      : never
    : never
  : never;

interface Example1 {
  foo: boolean;
  bar: { a: number; b: string };
  baz: Array<{ c: string[] }>;
}

type Example2 = {
  foo: boolean;
  bar: { a: number; b: string };
  baz: Array<{ c: string[] }>;
};

type Paths1 = ObjectPaths<Example1>;
type Paths2 = ObjectPaths<Example2>;

const path1: Paths1 = "*";
const path2: Paths2 = "*";
const path3: Paths1 = "foo";
const path4: Paths2 = "foo";
const path5: Paths1 = "bar.a";
const path6: Paths2 = "bar.a";
const path7: Paths1 = "bar.*";
const path8: Paths2 = "bar.*";
// @ts-expect-error
const path9: Paths1 = "unknown";
// @ts-expect-error
const path10: Paths2 = "unknown";

const test1A: ObjectProperty<Example1, "foo"> = true;
const test1B: ObjectProperty<Example2, "foo"> = false;

const test2A: ObjectProperty<Example1, "bar.a"> = 1;
const test2B: ObjectProperty<Example2, "bar.a"> = 2;

const test3A: ObjectProperty<Example1, "bar.b"> = "foo";
const test3B: ObjectProperty<Example2, "bar.b"> = "bar";

const test4A: ObjectProperty<Example1, "bar.*"> = 1;
const test4B: ObjectProperty<Example2, "bar.*"> = "foo";

const test5A: ObjectProperty<Example1, "*"> = true;
const test5B: ObjectProperty<Example2, "*"> = { a: 1, b: "foo" };

// @ts-expect-error
const test6A: ObjectProperty<Example1, "unknown"> = 1;
// @ts-expect-error
const test6B: ObjectProperty<Example2, "unknown"> = true;

Right now:

export interface Foo extends XataRecord {
  property: string | null;
}

Improves DX:

export interface Foo {
  id: string;
  property: string | null;
}

export type FooRecord = Foo & XataRecord;

The SDK resolves the branch the first time it needs it. It goes one by one, until it finds a string or a function that resolves in a stiring. Example:

import {local, git} from '@xata.io/client-utils/branch-resolution'

branch: [process.env.XATA_BRANCH, local, git, 'main']

In this case local and git are async function that return string, undefined or null. The resolved branch name will be stored in the XATA_BRANCH env variable. If it's not defined, it'll resolve the functions sequentially. If none of them return a defined string, the branch name used will be main.

Functions:

• Vercel Serverless Functions (Node v16)
• CF Workers (V8)
• Netlify Edge Functions (Deno)

Sites:

• Vercel (Next.js)
• CF Pages (Remix)
• Netlify (SvelteKit)

Other:

• Rollup bundle with browser eval (like our SDK playground)
• WASM with Fastly Compute@edge

Ref: #20 (comment)

This line is not working for the following Cloudflare Worker config:

I was able to work around this by changing this.fetch to fetch at the above line. I attempted:

1. To manipulate the constructor in RestRepository to assign cloudflare workers fetch - Did not work
2. Printing and comparing fetch and this.fetch and they report as the same.
3. Adjusting tsconfig to ES6 instead of esnext to align with client and validate this wasn't the issue - Did not work

I am really not sure why this isn't working, the only thing I added beyond the template was itty-router and faunadb (replacing with xata). This was a problem with both local wrangler dev and deployed code.

Cloudflare Worker:

• Template: https://github.com/cloudflare/worker-typescript-template

nvmrc

lts/gallium

tsconfig:

{
  "compilerOptions": {
    "outDir": "./dist",
    "module": "commonjs",
    "target": "esnext",
    "lib": ["esnext"],
    "alwaysStrict": true,
    "strict": true,
    "preserveConstEnums": true,
    "moduleResolution": "node",
    "sourceMap": true,
    "esModuleInterop": true,
    "types": [
      "@cloudflare/workers-types",
      "@types/jest",
      "@types/service-worker-mock"
    ]
  },
  "include": ["src"],
  "exclude": ["node_modules", "dist", "test"]
}

package.json

{
  "name": "tasker-backend-cf",
  "version": "0.1.0",
  "main": "dist/worker.js",
  "scripts": {
    "build": "webpack",
    "format": "prettier --write  '*.{json,js}' 'src/**/*.{js,ts}' 'test/**/*.{js,ts}'",
    "lint": "eslint --max-warnings=0 src && prettier --check '*.{json,js}' 'src/**/*.{js,ts}' 'test/**/*.{js,ts}'",
    "test": "jest --config jestconfig.json --verbose",
    "xata-codegen": "xata pull && xata-codegen generate -o src/generated/XataClient"
  },
  "author": "Nabil Cheikh",
  "eslintConfig": {
    "root": true,
    "extends": [
      "typescript",
      "prettier"
    ]
  },
  "devDependencies": {
    "@cloudflare/workers-types": "^3.0.0",
    "@types/jest": "^26.0.23",
    "@types/service-worker-mock": "^2.0.1",
    "@typescript-eslint/eslint-plugin": "^4.16.1",
    "@typescript-eslint/parser": "^4.16.1",
    "@xata.io/codegen": "^0.2.2",
    "eslint": "^7.21.0",
    "eslint-config-prettier": "^8.1.0",
    "eslint-config-typescript": "^3.0.0",
    "jest": "^27.0.1",
    "prettier": "^2.3.0",
    "service-worker-mock": "^2.0.5",
    "ts-jest": "^27.0.1",
    "ts-loader": "^9.2.2",
    "typescript": "^4.3.2",
    "webpack": "^5.38.1",
    "webpack-cli": "^4.7.0"
  },
  "dependencies": {
    "@xata.io/client": "^0.2.2",
    "faunadb": "^4.5.2",
    "itty-router": "^2.4.10",
    "nanoid": "^3.3.1",
    "worktop": "^0.7.3"
  }
}

• Add a getFirst and getOne (where getOne only accepts filters on unique columns)

Context #127 (review)

• Add delimiter option (semicolons, tabs, spaces)
• Review different encodings (latin1)
• Add a non-interactive flag, make all commands to inquiry instead of erroring out, if flags are provided ignore the inquiry question (related to https://github.com/xataio/client-ts/pull/127/files#r864487665)
• Add a way to call interactively as an npm package (API)

We are adding js-doc comments to describe parameters, but we are not adding type information. Should we add it? Or modern tooling can use the typescript types and that should be it?

It'd be great to have documentation that appears in contextual help in vscode and such.

Also this would be great if at some point we generate docs automatically from the source code.

As far as I can see, there are no tests for the codegen part, which makes it prone to regressions and similar issues. Let's add unit tests to this to ensure predictability. I'm happy to do this myself since we're all otherwise quite busy.

Right now the codegenerator can generate JavaScript, but maybe it's a better option to generate TypeScript and then transpile it. That could even be better to ensure that tools such as vscode find the types correctly.

My only concern is how well the generated code looks, because the generated code is something people may look at. Anyway maybe we can use prettier after the transpilation?

cc @fabien0102

And add integration tests

Given that the Xata CLI creates a folder and puts multiple files in it (config.json, schema.json, and .gitignore), should we update the codegen CLI to receive an argument for the folder instead and then work with that?

I think this also might make sense since we’d like to use the config.json file according to #25.

We can paginate using offset or using a cursor. In both cases a size can be specified.

I suggest adding a new interface/type for cursor-based pagination:

interface Page {
  private query: Query
  private cursor?: string

  nextPage(size?: number)
  previousPage(size?: number)
  firstPage(size?: number)
  lastPage(size?: number)

  records: readonly XataRecord[]
}

And add optional arguments to getMany() for offset pagination:

getMany({
  size?: number,
  offset?: number
})

Usage examples:

// Pagination with offset/size
const users = await xata.db.users.select().getMany()
const users = await xata.db.users.select().getMany({offset: 10})
const users = await xata.db.users.select().getMany({offset: 10, size: 50})
const users = await xata.db.users.select().getMany({size: 50})

// Cursor-based pagination

// Repository implements Page
const page = await xata.db.users.select().nextPage()
const page = await xata.db.users.select().firstPage()
const page = await xata.db.users.select().lastPage()

// Passing a size
const page = await xata.db.users.select().nextPage(100)
const page = await xata.db.users.select().firstPage(100)
const page = await xata.db.users.select().lastPage(100)

// records is an array, not a promise
const users = page.records

// Pages are immutable. Their methods return new pagination objects
const page2 = page.nextPage()
const page2 = page.previousPage()
const page2 = page.firstPage()
const page2 = page.lastPage()

const page2 = page.nextPage(100)
const page2 = page.previousPage(100)
const page2 = page.firstPage(100)
const page2 = page.lastPage(100)

From the README:

xata-codegen generate [path to xata directory] -o generated/client.ts

If you do that in main, the result is a generated/client.ts.ts file. Note the duplicated extension

If you do

xata-codegen generate [path to xata directory] -o generated/client.js

The result is a generated/client.js.ts extension.

Previously the language was deducted from the extension. But now there's an optional --lang parameter that if it's not used, the extension the .ts extension is always added regardless of the extension the user provides. I haven't checked what happens if you use the param and also put an extension in the -o option.

My suggestion is to remove the --lang option and deduct the language from the extension.

cc @TejasQ

It's easy to reproduce the probem by runnning npm run build-example

I feel this will help users get started much faster without mixing too many concepts. We can add codegen as a "cherry on the cake" after.

I was just using the client and was a little confused because there was no columns option here:

I expected the same options as visible in the UI:

After looking into it a bit more, I learned that this is the API we have:

I find this non-idiomatic because the select semantic is nowhere to be found anywhere else in our products and thus feels like a new concept to learn, where the flow that would feel more natural would be

What drawbacks do we have from this approach?

I believe we are not correctly building the objects in create and update operations, the API responds only with the id and we're not using the body. (Related code)

[off-topic] Do you know how/if we can retrieve this description from the API?

I don't see it so far 😢

Originally posted by @fabien0102 in https://github.com/xataio/frontend-next/pull/753#discussion_r860616266

    const finalObjects = await Promise.all(response.recordIDs.map((id) => this.read(id)));

We need to use in: $any: []

It is a problem with CF workers (50 max subrequests):

Thinking a bit about customer feedback, a point of friction is having to run codegen after npm install @xata.io/codegen -D.

What if we run the command automatically as soon as it’s installed, which would then automatically run the Xata CLI to download a schema and config, eliminating the manual step?

I think this might not run with yarn, but we can add a note in the README for that.

Thoughts?

Two questions:

1. The SDK calls it a token, but the UI, CLI, and docs call it "API Key". Shall we rename it to apiKey in the SDK?

2. url is quite a generic name:

I wonder if we can call it something like workspaceUrl or apiBase or something? url doesn't exactly tell me what URL it wants.

Related to: https://github.com/xataio/xata/pull/583

This is how we instantiate the client (from the example):

const client = new XataClient({
  url: 'https://myworkspace-123abc.xata.sh/db/databasename:branchname',
  token: 'xau_1234abcdef',
  fetch: fetchImplememntation // Required if your runtime doesn't provide a global `fetch` function.
});

For consistency with the API and docs, the token param should actually refer to API key, so something like apikey would be more accurate.

It seems that in case the API key is not valid, you get only a nil when running it:

npx @xata.io/importer ./companies.csv --table companies                                                         1 ↵
✖ <nil>

They sometimes fail, sometimes don't after a retry. 🧐

I was using bulkInsertTableRecords and got this generic error from the SDK:

{ message: 'Network response was not ok', status: 400 }

Debuging I found printed the response from the server:

{
  errors: [
    {
      status: 400,
      message: 'invalid record: column [name]: type mismatch: expected string'
    },
    {
      status: 400,
      message: 'invalid record: column [name]: type mismatch: expected string'
    }
  ]
}

I guess our fetcher doesn't parse this correctly because it always expects a single error, not an array.

Not sure what is expected here:

I tried an object, but

Why does it need table if I'm already querying db.users?

I searched the README but couldn't find anywhere this is covered too 😢

If typescript does not find circular references this should not have a depth check

A couple of things that I found while dog-fooding:

• Errors returned by withStatus don't have a stack trace. I had an error and it was hard to know where it was coming from, because the backend error was very generic.
• The bulk API sometimes returns errors without a message: e.g. { status: 409 }, so we finally get an Unknown error. We should ignore those and get the error messages from the rest of operations.

The client is a 667 line file with a lot going on. Shall we abstract out and import the functions from this file to make iteration etc. a bit more approachable?

Thankfully it's unit tested so we won't break much with such a change. Let me know and I'm happy to do this. 🚀

We generate @property annotations. See https://github.com/xataio/client-ts/blob/main/codegen/example/xata.js#L4-L12

I'm wondering if we could/should generate a .d.ts file and importing the types instead. Like here

npm i @xata.io/codegen -D
npm i @xata.io/client
xata pull
npx xata-codegen -c xata/config.json

vs

npx https://app.xata.io/sdk/my-project/db/branch

Bit of a shame. It just fails instead of presenting help:

➜  test-sdk yarn xata-codegen
yarn run v1.22.17
warning ../package.json: No license field
$ /Users/tejas/Sites/LAB/test-sdk/node_modules/.bin/xata-codegen
Using schema file: /Users/tejas/Sites/LAB/test-sdk/xata/schema.json
Using output file: /Users/tejas/Sites/LAB/test-sdk/xata.ts

Could not read schema file at ./xata/schema.json
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

The delete, read and update action should not be valid as filter field.

I apologize–seems I naïvely mistakenly approved this because CI is fully red on main right now with the following output.

Now that we have a xata/config.json file (we didn't have it a few months ago), we can read the workspace id and the database name during code generation and inject it in the code.

Users should still be able to overwrite its value for a few reasons:

• So the URL is configured with mechanisms like env variables. Useful for example if for security reasons somebody wants to run a codebase in a different workspace/database, or because it's a forked open source project and they don't want to change the generated code every time just to change the workspace/database, etc.
• For us, to test our generated code against localhost or staging.

So far I've been doing npm publish manually, but we should automate that.

Is the url when instantiating the client intended to be a base URL? Or a specific URL for databases? Can one do workspaces operations with it? I'm not sure I understand this:

It suggests one instance of a client per DB Branch?

• Use GitHub Actions to run tests in every push.
• Add secrets to run these tests against xata.io/sh.
• Prevent merging a PR if tests are not passing.

Generally I think it makes sense to make it very clear that the XataClient is generated and not intended to be manually changed, so I'm wondering if it's a good idea to make this even more explicit with a directory by default. Of course, users can choose other output paths and it'll follow nicely.

We could even add a banner on top of the file stating something like this.

Would that be good developer experience?

I'm doing

const xata = new XataApiClient({ apiKey });

and tsc is telling me that I'm missing a fetch implementation. I'm in Node.js 14 so I'll need to provide one, but some runtimes won't need this, so it should not be a required field.