Draft
Conversation
When this code was first written in 2017, Buffer methods were not supported across all node versions. As of 2026, this is no longer the case - we delete the custom implementations and re-export the proper buffer methods to avoid tedious refactoring of all internal usages.
This commit (almost) automatically converts all class functions in metadata module to class syntax.
This is a update based on the discussion in scylladb#282. Initially, the test was assuming that the fist host is the one provided. We decided to not uphold this assumption. As a result this commit updates the test to properly check for all combinations of host orders, by using Chai's `expect` instead of `assert`. I also added a small node about this in the migration guide.
HostMap.toJSON had a custom implementation, that was used when Object.fromEntries was not available. This is supported since Node 12, so we can safely remove this code, and rely fully on Object.fromEntries. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/fromEntries#browser_compatibility
Before, some members of the encoder were set in the separate setEncoders and defineInstanceMembers functions. This reduced the readability and type safety of the code. This commit refactors all of those members to be set in a single class, the Encoder inherits from. This allows better type readability across the encoder, and should improve readability. Historically, the separation between Encoder and it's members likely existed because previously Encoder was exposed in the API (it no longer is) and such separation allowed to keep members of this class private (invisible in the public API docs). This approach hoverer reduced also internal documentation readability, which is now fixed. Currently those members are kept separate to easier code reading / editing. EncoderMembers focuses on encoding/decoding methods for individual types, while Encoder "public" (only for the driver) part of the API.
Why we want to make this change.
Exporting from TS under a namespace meant, that when importing the value,
we had different internal API.
Ex. when importing `const types = require("./types");`
to access `Integer` we had to type:
- `types.Integer` when importing JS code
- `types.types.Integer` when importing TS code.
This didn't impact the public API due to the way it was re-exported in main.d.ts.
Hover this caused the built-in tools in VScode to incorrectly recognize
the imported types (VScode tried to import the TS file,
while in practice js file was imported). This commit updates
the internal part of TS api to fix this problem, without changing
the external TS api, fixing the internal type recognition in TS.
This API is a 1:1 mapping of the Table metadata exported by the rust driver.
The goal of those classes without constructors is to provide type definition,
visible and understandable for users. Metadata objects can be initialized
with object construction syntax (ex. {partitionKey: ['id']})
and returned from type annotated functions, without any penalty for lack
of constructors.
This API is a 1:1 mapping of the Materialized View metadata exported by the rust driver.
The goal of those classes without constructors is to provide type definition,
visible and understandable for users. Metadata objects can be initialized
with object construction syntax (ex. {partitionKey: ['id']})
and returned from type annotated functions, without any penalty for lack
of constructors.
This API is a 1:1 mapping of the Table metadata exported by the rust driver.
This includes a Strategy class used in the table metadata.
The goal of those classes without constructors is to provide type definition,
visible and understandable for users. Metadata objects can be initialized
with object construction syntax (ex. {partitionKey: ['id']})
and returned from type annotated functions, without any penalty for lack
of constructors.
This removes all internals of metadata classes, leaving only the public API without any implementation. This simplification is done, to better understand changes to the public API that will be done in the following commits.
This was a metadata specific to DSx service
The plan is to let the rust driver handle all metadata fetching. This means we will be able to retrieve all the metadata in a sync way. This commit updates the metadata class API to expose all endpoints in a sync way. This is meant to split the API changes into smaller parts, to better understand the differences between the old and new API.
This was used as a Base class for Materialized View and Tables metadata
This is a no longer used part of the code that wasn't exposed in the public API
I see no such description (or any comment) in the linked issue |
Contributor
Author
|
Linked wrong issue, should have been #261 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[Note: This PR is based on the adespawn:drop-more-deprecated branch. I will open this PR for review once #405 is merged]
After some discussion with @wprzytula, we have decided that the existing API for the metadata is not worth keeping.
The problems with the DSx API are described in #261.
This PR introduces new API for the tables and keyspaces metadata. With this PR I only introduce the classes that will be returned from the metadata API. The endpoints used for retrieving this metadata will also be updated (probably in the separate PR). Retrieving of the metadata from Rust side will also come in the following PR
Refs: #261, #70