-
Notifications
You must be signed in to change notification settings - Fork 7
Internal documentation #442
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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) |
| 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? |
| 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. | ||
|
|
||
| ## 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing:
|
||
|
|
||
| ### 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please recall how and why this is faster than use of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing code example + |
||
|
|
||
| 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( | ||
|
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. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing examples.