Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 7 additions & 12 deletions MAINTENANCE.md
Original file line number Diff line number Diff line change
Expand Up @@ -92,19 +92,13 @@ Remember to run `npm i` in the `examples/` and `benchmark/` directories once you
2. Bump the package version. Remember to update the version in `package-lock.json` in
the main directory, `examples/`, and `benchmark/`. You can do this by running `npm i` in all 3 directories
(see [example commit](https://github.com/scylladb/nodejs-rs-driver/pull/363/changes/41250609737052975129c7514439869324478008) on how to do that).
3. Create a new tag.
4. Ensure the extended CI passes.
5. Create release notes on GitHub. The version tag must match version from `package.json` with `v` prefix (for example: `v0.2.0`).
3. Add the new tag to the documentation configuration (see #470)
4. Create a new tag.
5. Ensure the extended CI passes.
6. Create release notes on GitHub. The version tag must match version from `package.json` with `v` prefix (for example: `v0.2.0`).
Once you publish release notes, CI action will trigger automatically. This action will build and publish the npm package.
6. Once the CI action finishes, check if it succeeded. If it failed, you will have to fix the underlying issue, and re-run the CI action.
7. Verify that the new release is visible at [npmjs site](https://www.npmjs.com/package/@scylladb/driver).
8. Test the package, by installing it directly from npm. Go to `examples` directory, in `package.json` update the line:
`"@scylladb/driver": "file:./../"`
to:
`"@scylladb/driver": "<just-released-version>"`,
then run the following command:
`npm i && node ./runner.js`
<!-- The last step can potentially be set up as a CI action step. -->
7. Once the CI action finishes, check if it succeeded. If it failed, you will have to fix the underlying issue, and re-run the CI action.
8. Verify that the new release is visible at [npmjs site](https://www.npmjs.com/package/@scylladb/driver).

### Updating packages

Expand Down Expand Up @@ -141,6 +135,7 @@ When converting existing code from JS to TS you can do it in 2 parts:

- Ensuring type safety within JS code,
- Converting JS to TS.
- Ensure empty `.npmignore` is present in the directory you just introduced the typescript file in.

The repository is set up in a way that supports files at all 3 conversion steps (including fully unconverted files).

Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# ScyllaDB Node.js-RS Driver
# ScyllaDB Node.js RS Driver

![test workflow](https://github.com/scylladb-zpp-2024-javascript-driver/scylladb-javascript-driver/actions/workflows/integration-tests.yml/badge.svg?branch=main)
![quality workflow](https://github.com/scylladb-zpp-2024-javascript-driver/scylladb-javascript-driver/actions/workflows/check-docs.yml/badge.svg?branch=main)
Expand Down
29 changes: 29 additions & 0 deletions docs/source/internal/SUMMARY.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,35 @@
# Summary

## Used shortcuts

Within this document (and internal code documentation) I may use informal names for different products.
Here you can find a list of those names in all their variants, with the versions we decided on in user-facing parts of the driver.

JS - JavaScript
TS - TypeScript

Node, NodeJs. Official: [Node.js](https://nodejs.org/)

Napi, NodeAPI, Napi-rs: see [napi.md](./napi.md#distinction). Official [Node-API](https://nodejs.org/api/n-api.html) and [NAPI-RS](https://napi.rs/).

DSx: Official: [DataStax](https://www.ibm.com/products/datastax)

DSx driver, old driver, DataStax driver, cassandra driver. Official:
[cassandra-driver/cassandra-nodejs-driver](https://github.com/apache/cassandra-nodejs-driver)
(The version depends on whether the language of the driver is obvious).
This driver was initially developed by DSx and was transferred to Apache during development of this driver.
For this reason some places may use the name referring to DSx. Any remaining public documentation should refer to this driver as
`cassandra-driver`, with possible annotation like: `formerly known as DataStax Node.js Driver` (see main [README.md](../../../README.md)).

this driver, new driver, scylladb-driver, scylladb-driver-alpha. Official: ScyllaDB Node.js RS Driver / scylladb-driver-alpha / nodejs-rs-driver
(standalone name / package name / repository name). Confusing? No worries, this repo probably breaks this convention quite a lot.

## Sections

- [Errors](./error-throwing.md)
- [ParameterWrapper](./parameter-wrapper.md)
- [Query options overview](./query-options.md)
- [Logging](./logging.md)
- [Napi](./napi.md)
- [Miscellaneous](./miscellaneous.md)
- [On types](./on-types.md)
72 changes: 72 additions & 0 deletions docs/source/internal/miscellaneous.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
# Miscellaneous stuff

## DataStax API

DSx driver had some external APIs that were related to DSx-specific features:

- Custom auth providers
- The entire datastax & geometry modules
- Some client / query options

How did we handle those?

For functions/methods, we implemented stubs in JS code:

```js
/**
* @deprecated Not supported by the driver. Usage will throw an error.
*/
class Vertex {
constructor() {
throwNotSupported("Vertex");
}
}
```

See [`structure.js`](../../../lib/datastax/graph/structure.js). The goal of those stubs is to loudly inform
JS users of the driver that our driver does not support those features. This approach, while good for users transitioning
from the old DSx driver, may provide some unnecessary noise for new users. When stabilizing the API in 1.0, those
stubs can be removed, with proper documentation for transitioning users. (If there is such a need,
you could create a temporary transition version for users transferring from the DSx driver, with those stubs enabled).

For the TS users of the driver, it was enough to remove those endpoints from the TS files.
When someone attempts to use those features, they will get a compilation error,
meaning we achieved the goal of explicitly informing the user of the deprecation of those endpoints.

When it comes to no longer supported options, we have a very similar approach:
When a JS user provides a no-longer-supported option,
[we throw an error](https://github.com/adespawn/nodejs/blob/ee439f78ed4b4e6c0a73b8aa2f649e45493ae1c5/lib/client-options.js#L971-L978).
Those options are not mentioned in documentation, avoiding the noise for new users.
When it comes to TS users, deleting those options from the TS API is again enough.

## Personal vendettas

Well, maybe not exactly personal, but still. This list is composed of small and big annoyances with the current state of the driver.

### API overloads

```js
* @example <caption>Overloads</caption>
* client.eachRow(query, rowCallback);
* client.eachRow(query, params, rowCallback);
* client.eachRow(query, params, options, rowCallback);
* client.eachRow(query, params, rowCallback, callback);
* client.eachRow(query, params, options, rowCallback, callback);
```

From this [piece of code](https://github.com/adespawn/nodejs/blob/ee439f78ed4b4e6c0a73b8aa2f649e45493ae1c5/lib/client.js#L418-L423).

Allowing for overloading API in this way is (while perfectly fine by JS standards) very annoying when it comes to ensuring type safety.
We have to guess based on the value types what overload the user is providing us with. But touching it will break the API.
Is someone using those overloads? Idk. Will it be hard for users to remove usages of those overloads? Probably not.
Feel free to re-visit this when creating 1.0.

### Long

The driver was initially developed when BigInt [was not a thing](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt#browser_compatibility).
This means there are internal (and external) parts of the driver that use [Long](https://www.npmjs.com/package/long).
This is the only JS dependency (excluding development dependencies) that the driver uses.
While internal pruning of Long should be (probably annoying but still) possible, removing it from external parts of the driver
may break some APIs. There is an issue related to this: [#41](https://github.com/scylladb/nodejs-rs-driver/issues/41).
This topic would need to be re-visited before 1.0. You would have to decide how to approach this: keep Long as a legacy part of the API,
or fully remove it?
161 changes: 161 additions & 0 deletions docs/source/internal/napi.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
# Napi (and napi-rs)

## Distinction

Node-API and napi-rs are two different things. When encountering just "napi" **in the documentation** (or PRs/issues),
what is meant in most cases is Node-API (although there may be some exceptions, especially in old docs).
However, when you encounter napi **in the code** (mostly Rust code, the napi crate), this refers to napi-rs.

In napi-rs there are different kinds of interfaces. Some of the interfaces napi-rs exposes are just 1-to-1 mappings from Node-API (mostly `napi::sys`).
Then there are low-cost abstractions over Node-API, like `(To/From)NapiValue` traits and `napi::Env`.
Then there are high-cost interfaces. Some of those add overhead that we would anyway need to introduce (like **sync** functions: functions with the `#[napi]` macro).
Others add significant overhead that we can avoid by using lower-abstraction-level approaches to achieve sometimes very significant performance improvements.
This often comes at the cost of some limitations, however those limitations rarely pose a problem for our use cases.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing examples.


## Wrappers

There may be cases where you have some Rust value you want to work on, but the JS code does not need to have access to the members of such value,
only its methods (think: [client session](https://github.com/adespawn/nodejs/blob/452f2acd2d8794161a1866c4b336df75c038cd22/src/session.rs#L29-L32)).
In those cases you can create a wrapper that keeps the value private to Rust code and exposes methods through napi.
Comment on lines +15 to +19

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing examples.


## Our interfaces

The main benefit of high-cost napi-rs interfaces is that they provide abstractions that very significantly reduce required verbosity.
This section briefly describes the available internal interfaces and their use cases:

The napi-rs offers [napi-rs classes](https://napi.rs/docs/concepts/class). They offer a quite easy to understand interface.
Using them is however [very](https://www.scylladb.com/wp-content/uploads/image10-5-768x768.png) [inefficient](https://www.scylladb.com/wp-content/uploads/image2-23-768x768.png),
as we [have](https://github.com/scylladb/nodejs-rs-driver/pull/182) [discovered](https://github.com/scylladb/nodejs-rs-driver/pull/181).

So why is it so slow?

Creating an [Object](https://nodejs.org/api/n-api.html#napi_create_object) through Node-API is quite slow, and possibly even slower than doing the same from JS code
(note: this is my recollection of things that happened over a year ago as of writing this documentation). Additionally, when [updating](https://github.com/scylladb/nodejs-rs-driver/pull/291)
to [napi-v3](https://napi.rs/docs/more/v2-v3-migration-guide), our existing use cases stopped working due to
[some problems](https://github.com/scylladb/nodejs-rs-driver/pull/291/commits/7c41208dc106f7ff20e1d0ffb9d33ecedd69e1ce) with BigInt.

To solve this problem, we have come up with a few workarounds, depending on the specific situation. For objects containing information, we design them
not to have any methods on them. This is necessary since our solutions introduce some limitations at the benefit of better usability / performance in our specific use cases.
Comment on lines +37 to +38

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing:

  • justification of no methods in information-carrying objects;
  • what limitations? why?
  • what specific use cases?


### To Napi Value

This section describes solutions for converting values from Rust to JS code.

#### Solutions for performance

As mentioned above, creation of JS [Objects](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object) is quite expensive.
What we have come up with, when returning multiple values from performance-critical paths, is to return a tuple ([array](https://nodejs.org/api/n-api.html#napi_create_array))
of values we want to return.

You can find examples of such use cases in the [Rust part](https://github.com/scylladb/nodejs-rs-driver/blob/2e04a1701e4cfa039bac35c96d8b84b3a5f93867/src/paging.rs#L53-L73)
of the code, as well as the [js parsing](https://github.com/scylladb/nodejs-rs-driver/blob/f4d6a68641bec162f066641b599eb31ec6efc0fa/lib/client.js#L354-L377)
of such values.

#### Clean use for non critical paths

Sometimes we want to return objects with [clear structure](https://github.com/adespawn/nodejs/blob/452f2acd2d8794161a1866c4b336df75c038cd22/src/metadata/host.rs#L5-L12),
where performance is not so critical (think: metadata). In this case helpers defined in [to_napi_obj.rs](../../../src/utils/to_napi_obj.rs) come to play.
They allow you to convert:

- Rust structs and enums with values in variants to JS
- Rust maps (String -> Any) into JS objects

Details on how to use them are present in the code docs for that file.
Those helpers are a specialized replacement for [napi-rs classes](https://napi.rs/docs/concepts/class).
They provide slightly better performance than napi-rs classes (since they do a bit less stuff on the returned object) and a reasonable user interface.
Comment on lines +54 to +65

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please recall how and why this is faster than use of #[napi] or #[napi(object)] macro.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you say that performance is only slightly better, how is this self-baked solution better than napi-rs classes?


### From Napi Value

Similar to the previous section, we have helpers for converting JS objects to Rust structs. To see more, go to [from_napi_obj.rs](../../../src/utils/from_napi_obj.rs).
This macro provides a much cleaner interface than [napi-rs classes](https://napi.rs/docs/concepts/class).
Compared to napi-rs classes, it allows you to convert arbitrary JS objects with the desired structure, rather than only objects created through the napi-rs layer.

When using structs from this macro, all fields are converted from JS to Rust. This means that when you have a struct with a high number of fields but use only a few of them,
you will still pay the cost of converting all unused fields.

### Casync (async bridge)

This is an open improvement that attempts to cut the very high CPU cost of synchronization in async functions.
You can see the benchmarks for this feature in [#414](https://github.com/scylladb/nodejs-rs-driver/pull/414).
TODO: finish this section once #414 is merged.

### JSResults

We use a custom JSResult for results that may end with an error. This replaces napi-rs Results, as those do not allow for custom errors.
We need custom errors since the errors returned by the Rust driver contain additional information. For now we just pass the error name.
Comment on lines +84 to +85

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional information? Additional to what?

We then need to use JSResult over regular Result since we cannot implement the ToNapiValue trait on a regular Result.
This complicates the code slightly but is the solution we found with the lowest verbosity.

## Typing

When you pass values between JS and Rust it's nice to have it typed in some way, to improve both readability and correctness of the code.
When using napi-classes or node-built in type representations (integers, bigInt, arrays, `Result`*, strings, buffers), in regular sync function,
Napi-rs will generate type annotations correctly in the `index.d.ts` file upon compilation of the code.

\* result type is not "visible" across, since napi-rs does not annotate what the function can throw, but is properly handled as a passthrough type.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing code example + Result<T> example. Let's say that T is that passthrough type.


However, when using either custom (To/From)NapiValue implementation, one of the helper macros or casync functions, you will need to do a bit more work to properly annotate the types.
Generally there are two things you need to do, in order to properly annotate those uses. What you need to do depends on the case.

- [`custom-types.d.ts`](../../../src/custom-types.d.ts): This file defines custom types. This means when you define new type, that you accept as argument or return from rust function,
and it is **not** napi-class, you have define its JS(TS) type there.
This file will then tell the JS/TS parser what does the magic type annotated in Rust function ex. `PagingResultWithExecutor` means.

So in the Rust code you just return `PagingResultWithExecutor`:

```rs
// Simple version
#[napi]
pub async fn query_single_page_encoded(
Comment thread
adespawn marked this conversation as resolved.
&self,
...
) -> Result<PagingResultWithExecutor> {


// Full version with manual type override (see next point)
#[napi(ts_return_type = "Promise<PagingResultWithExecutor>")]
pub async fn query_single_page_encoded(
&self,
...
) -> JsResult<PagingResultWithExecutor> {
```

Napi-rs then annotates the TS definition of this function as returning `PagingResultWithExecutor`:

```ts
export declare class SessionWrapper {
...
querySinglePage(...): Promise<PagingResultWithExecutor>
}
```

The custom definition in the `custom-types.d.ts`:

```ts
export type PagingResultWithExecutor = [PagingStateWrapper | null, QueryResultWrapper, QueryExecutor]
```

provides the meaning for this type. This is something you need to add manually to `custom-types.d.ts` file
for every new type you add. This file will then be prepended to `index.d.ts` (used by JS/TS engine when parsing types in VScode) at the rust compilation time.

Note: this could be possibly automated with `pub trait TypeName` napi-rs trait.
- Manual type overrides: When the napi-rs cannot properly deduce the JS name, you need to manually override the type in the endpoint that utilize it.
By default, when you have a type `T` in Rust, and you were to have rust function return value of that type, it will be annotated in TS as `T`.
Similarly, if you have a generic type: `G<T>` it will be annotated as `G<T>` in TS.

Here comes the problem: We have a type `JsResult<T>`, which should be opaque (the same way regular `Result<T>` is opaque) - meaning it should be annotated as
`T` in TS (instead of the default `JsResult<T>`). Unfortunately you cannot handle it automatically.

To properly annotate such functions you need to use napi-rs `ts_return_type` for every function that returns such type.
When using this tag, you need to specify the full type like this: `#[napi(ts_return_type = "Promise<PagingResult>")]`
(meaning you need to remember about Promise part for async functions).

It's annoying, but allows to properly annotate the functions.
You can see me question about this feature on the [napi-rs server](https://discord.com/channels/874290842444111882/874290843262021705/1505966134615085088).

## The napi problem

The napi-rs library is not perfect. We have already [found and fixed](https://github.com/napi-rs/napi-rs/issues?q=author%3Aadespawn) multiple bugs in the library.
The library is developed with heavy LLM usage. The quality and readability of the code in some places is less than ideal.
However, this is still the best option we have. To reduce the impact of problems with the napi-rs code, we avoid using high-abstraction parts of napi-rs,
and where possible switch to self-written, specialized abstractions.
Loading
Loading