Client basics #1

aoberoi · 2021-04-21T22:56:08Z

Goals

Establish a project / file structure
Establish build configuration (TypeScript)
Implement some basic client features (the API was established in an internal RFC)
- Initializing a client instance (with options)
- Sending requests via endpoint methods (only two representative methods are currently available)
- Sending requests via a generic request interface
Establish some type definition patterns that we can support in the future with automatic generation.
Establish some testing and linting configuration

The following items remain:

~~Add the remaining endpoint methods~~ I decided to add this in a follow up PR in order to help this change land sooner, and make some incremental progress.

Other client features will be implemented in subsequent PRs, such as implementing error types.

Feedback

At this point, I'm seeking input on the following:

Does the project / file structure make sense?
Are you happy with the build, testing, and linting config?
Are there any red flags in the way the Client is implemented so far?
Any thoughts regarding how the type definitions are structured?

aoberoi · 2021-04-21T22:58:15Z

src/api-endpoints.ts

+ * Notion API Endpoints
+ *
+ * This file contains metadata about each of the API endpoints such as the HTTP method, the parameters, and the types.
+ * In the future, the contents of this file will be generated from an API definition.


This file and src/api-types.ts contain a lot of information that we'd like to generate from the internal source. For now, the plan is to do this manually. In the future the generation would occur in two steps. First, the internal source would be used to generate an OpenAPI definition (accompanied with JSON Schema for object types). That output would be shared publicly for many use cases beyond API clients (e.g. Postman, testing, etc). Next, the OpenAPI definition would be used to generate these files in this client, and similar code for the Python client.

aoberoi · 2021-04-21T22:59:33Z

src/Client.ts

+   * @param body
+   * @returns
+   */
+  public async request<Response extends {}>(path: string, method: Method, query?: QueryParams, body?: {}, auth?: string): Promise<Response> {


In the future, I would like to include a more type-aware overload of this method. The goal would be for the return type to depend on the individual arguments (without any generic inputs at the call site). It might look something like the following (but QP and B should also be constrained to more specific types based on P and M):

public async request<P extends string, M extends Method, QP extends QueryParams, B extends {}>(path: P, method: M, query?: QP, body?: B): Promise<APIResponse<P, M>>

let’s use named arguments

Better to have object as the type bound

yeah that method signature is getting long / illegible. good call on named args!

regarding the type constraint on Response (the diff above is outdated) i'm getting a hassle from the linter. when i constrain to Response extends object i get a violation of the @typescript-eslint/ban-types rule:

Don't use `object` as a type. The `object` type is currently hard to use ([see this issue](https://github.com/microsoft/TypeScript/issues/21732)). Consider using `Record<string, unknown>` instead, as it allows you to more easily inspect and use the keys.

i believe Response extends {} is actually the same thing as above, but the same rule says that's a violation because its unclear:

Don't use `{}` as a type. `{}` actually means "any non-nullish value". - If you want a type meaning "any object", you probably want `Record<string, unknown>` instead. - If you want a type meaning "any value", you probably want `unknown` instead. - If you want a type meaning "empty object", you probably want `Record<string, never>` instead.

if i take its advice and use Response extends Record<string, unknown> then the linter is cool but i get a type check error on L75 (where I use DatabasesRetrieveResponse as Response):

Type 'DatabasesRetrieveResponse' does not satisfy the constraint 'Record<string, unknown>'. Index signature is missing in type 'DatabasesRetrieveResponse'.

that seems wrong because i don't want to introduce an index signature to DatabasesRetrieveResponse. if i did, then when a developer uses a response and misspells a property name (e.g. response.ib, they wouldn't necessarily get a compiler error, it would just evaluate to unknown which can slide through).

in conclusion, i left off the type constraint because i think the right way to solve this problem is with an overload of the method signature that constrains method and path to types that match our list of endpoints, and then infers the response type based on that. this function signature would stick around for the developers who really know that their response is going to be a specific type (that maybe the client doesn't know about).

thank you for reading my TED talk (tried to be as concise as i could, sorry). do you agree with that last bit? any other ideas?

ps. i think the linter's recommended rules are reasonable, so i'm not super in favor of turning them off or reconfiguring them. maybe you can convince me otherwise?

In this case, I think the rule is being silly, but, oh well. It seems that object is the correct thing to do here per the compiler, but it doesn't lint. If you wan this repo to look like "typescript on average" then go for the linting experience and leave off the type constraint

aoberoi · 2021-04-21T23:00:28Z

src/api-endpoints.ts

+interface DatabasesRetrieveBodyParameters {}
+
+export interface DatabasesRetrieveParameters extends DatabasesRetrievePathParameters, DatabasesRetrieveQueryParameters, DatabasesRetrieveBodyParameters {}
+export interface DatabasesRetrieveResponse extends NotionDatabase {}


In the future, *Response types can become generic on the *Parameters type, so that the type system can support any variation in response based on certain kinds of params (conditional types). This kind of relationship may prove difficult or impossible to express in an OpenAPI definition file, so we are intentionally saving this exploration for later.

src/Client.ts

cwang314 · 2021-04-22T00:34:33Z

src/Client.ts

+        databasesRetrieve.method,
+        pick(args, ...databasesRetrieve.queryParams),
+        pick(args, ...databasesRetrieve.bodyParams),
+        args.auth,


it looks like we never actually use this.#auth?

it's used in Client.authAsHeaders()

aoberoi · 2021-04-22T15:02:01Z

i've added some basic linting, but we still likely want to customize and add a few more rules. i don't consider getting the rules completely right as blocking for this PR. we can follow up on that.

ava was chosen as a test runner (well-maintained, excellent typescript support) adding this config required slightlying changing the build output config too. the distributable artifacts are now one level deeper, so that they can remain in a separate directory from the test artifacts. for clarity, the output directory was renamed from `dist` to build`, and within build, there is `src` and `test`. only `build/src` is meant for packaging (and that is reflected in the "main" property of package.json).

…ams were getting chunky

aoberoi · 2021-04-22T18:33:56Z

@justjake adjusted Client.request() to take named params as you suggested.

justjake

left some comments. seems legit

justjake · 2021-04-22T22:19:57Z

ava.config.js

@@ -0,0 +1,9 @@
+export default {


never used this. we use mocha in the main app.

me neither actually. but the allure of parallelized tests in worker threads and strong typing was too much for me to turn down. and packages by @sindresorhus are generally awesome.

justjake · 2021-04-22T22:20:51Z

package.json

+  "author": "",
+  "license": "MIT",
+  "dependencies": {
+    "got": "^11.8.2"


initially i was dubious of got since it's 90kb to make HTTP requests even though HTTP client is built into the standard library, but it seems to do a lot of stuff and generally looks useful.

You should be careful to isolate our return and exception types from Got's -- Got throws GotHttpError or similar; we should be careful about consumers needing to install Got to handle our exceptions.

agreed. i made type aliases for Method and QueryParams in order to create that separation. error types are still a TODO.

justjake · 2021-04-22T22:44:20Z

src/Client.ts

+   * @param level The level for this message
+   * @param args Arguments to send to the console
+   */
+  private log(level: LogLevel, ...args: any[]) { // eslint-disable-line @typescript-eslint/no-explicit-any


prefer unknown

justjake · 2021-04-22T22:45:04Z

src/Client.ts

+    /**
+     * Retrieve a database
+     */
+    retrieve: (args: WithAuth<DatabasesRetrieveParameters>): Promise<DatabasesRetrieveResponse> => {


justjake · 2021-04-22T22:45:32Z

src/Client.ts

+ * Type aliases to support the generic request interface.
+ */
+type Method = 'get' | 'post' | 'patch';
+type QueryParams = GotOptions['searchParams'];


i'd like to try keeping non-exported things at the bottom. that way when a consumer is reading the source, the parts they care about at closest to the top.

it's kinda arbitrary that these are not exported, since they are accessible via the interface of Client. i just chose not to export as a small signal that these types aren't that important or useful on their own.

justjake · 2021-04-22T22:52:11Z

src/Client.ts

+    this.#logLevel = options?.logLevel ?? LogLevel.WARN;
+
+    const prefixUrl = (options?.baseUrl ?? 'https://api.notion.com') + '/v1/';
+    const timeout = options?.timeout ?? 60;


justjake · 2021-04-22T22:54:06Z

tsconfig.json

+    // Node LTS Fermium (14.x) has mostly complete support for ES2019 (as reported by https://node.green/)
+    "target": "ES2019",
+    "module": "commonjs",
+    // "esModuleInterop": true,


we could turn this on, but it just emits a little extra and possibly unnecessary code. we're just as good as long as src/index.ts only emits named exports, because those are 1:1 with CommonJS.

justjake · 2021-04-22T22:54:37Z

tsconfig.json

+
+    // Strict mode
+    "strict": true,
+


Shall we turn on isolatedModules: true?

I also commonly have

"moduleResolution": "node", "resolveJsonModule": true,

not sure if wanted/needed

of those, i think we might want resolveJsonModule when i start reading package.json for the module's version number to stick it in User-Agent.

justjake · 2021-04-22T22:56:08Z

tsconfig.json

+
+  "include": [
+    "src/**/*",
+    "test/**/*",


this will output compiled tests. that's okay, but often test runners can run from the .ts files directly

ava supports both: https://github.com/avajs/ava/blob/main/docs/recipes/typescript.md#enabling-avas-support-for-typescript-test-files

this way was just much simpler. i think the ts-node option becomes more attractive when there's more involved in the build step (babel, webpack, etc), which i hope to avoid.

justjake · 2021-04-22T22:56:45Z

package.json

+  "engines": {
+    "node": ">=14.15.0"
+  },
+  "main": "./build/src",


shall we exclude the tests from packaging?

yaaa good call. i should add a files key or an npmignore.

@justjake

thanks @justjake

…eat-basic-functionality

Add block type example

aoberoi added 2 commits April 21, 2021 15:41

project metadata and config

bddebe6

adds basic client functionality

fe969fa

aoberoi commented Apr 21, 2021

View reviewed changes

src/Client.ts Outdated Show resolved Hide resolved

move tsc to devDeps

83998dc

cwang314 reviewed Apr 22, 2021

View reviewed changes

aoberoi added 2 commits April 21, 2021 22:03

fix type check issue with pick() helper

c890485

adds basic linting configuration and updates code to pass rules

d6c81e8

aoberoi force-pushed the feat-basic-functionality branch from 3692995 to d6c81e8 Compare April 22, 2021 15:00

aoberoi marked this pull request as ready for review April 22, 2021 16:10

alicial requested review from pllnk and justjake April 22, 2021 16:51

applies named arguments pattern for Client.request(), poisitional par…

c33ce78

…ams were getting chunky

aoberoi added 3 commits April 22, 2021 12:20

Merge branch 'main' into feat-basic-functionality

7c7a95b

Merge branch 'main' into feat-basic-functionality

14b6de2

Merge branch 'main' into feat-basic-functionality

97039e7

justjake approved these changes Apr 22, 2021

View reviewed changes

aoberoi added 2 commits April 22, 2021 17:33

type safety and style improvments, adjust timeout option to milliseconds

1d2500c

thanks @justjake

Merge remote-tracking branch 'origin/feat-basic-functionality' into f…

8e734dd

…eat-basic-functionality

aoberoi merged commit fa21dd6 into main Apr 23, 2021

aoberoi deleted the feat-basic-functionality branch April 23, 2021 00:43

aoberoi mentioned this pull request May 13, 2021

Add all named endpoints and types #20

Merged

lao9 mentioned this pull request Jul 21, 2021

[examples] test and improve glitch examples #158

Merged

jessmitch42 referenced this pull request in jessmitch42/notion-sdk-js Aug 14, 2023

Merge pull request #1 from jessmitch42/add-block-type-example

62a9b77

Add block type example

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client basics #1

Client basics #1

aoberoi commented Apr 21, 2021 •

edited

aoberoi Apr 21, 2021 •

edited

aoberoi Apr 21, 2021 •

edited

justjake Apr 22, 2021

aoberoi Apr 22, 2021 •

edited

justjake Apr 22, 2021

aoberoi Apr 21, 2021

cwang314 Apr 22, 2021

aoberoi Apr 22, 2021

aoberoi commented Apr 22, 2021

aoberoi commented Apr 22, 2021

justjake left a comment

justjake Apr 22, 2021 •

edited

aoberoi Apr 23, 2021

justjake Apr 22, 2021

aoberoi Apr 22, 2021

justjake Apr 22, 2021

justjake Apr 22, 2021

justjake Apr 22, 2021

aoberoi Apr 23, 2021

justjake Apr 22, 2021

justjake Apr 22, 2021

aoberoi Apr 23, 2021

justjake Apr 22, 2021

justjake Apr 22, 2021

aoberoi Apr 23, 2021 •

edited

justjake Apr 22, 2021

aoberoi Apr 23, 2021

justjake Apr 22, 2021

aoberoi Apr 23, 2021

Client basics #1

Client basics #1

Conversation

aoberoi commented Apr 21, 2021 • edited

Goals

Feedback

aoberoi Apr 21, 2021 • edited

Choose a reason for hiding this comment

aoberoi Apr 21, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aoberoi Apr 22, 2021 • edited

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

aoberoi commented Apr 22, 2021

aoberoi commented Apr 22, 2021

justjake left a comment

Choose a reason for hiding this comment

justjake Apr 22, 2021 • edited

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

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

Choose a reason for hiding this comment

aoberoi Apr 23, 2021 • edited

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

aoberoi commented Apr 21, 2021 •

edited

aoberoi Apr 21, 2021 •

edited

aoberoi Apr 21, 2021 •

edited

aoberoi Apr 22, 2021 •

edited

justjake Apr 22, 2021 •

edited

aoberoi Apr 23, 2021 •

edited