Open
Conversation
adespawn
force-pushed
the
drop-more-deprecated
branch
from
1f316ac to
a812258
Compare
There was a problem hiding this comment.
Pull request overview
This PR bundles a few cleanup changes across the Node.js driver, including modernizing Buffer allocation helpers, fixing the all-tests script wiring, and converting several metadata model modules to ES classes as groundwork for upcoming Metadata API work (#261).
Changes:
- Update
package.jsontest script to runintegrationandunit-not-supported. - Remove legacy Buffer allocation fallbacks and re-export modern
Buffer.*methods fromlib/utils.js. - Convert multiple metadata model constructors (table/view/index/function/aggregate/data-collection) from function constructors to ES classes.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Adjusts all-tests script to current test suite layout (integration, unit-not-supported). |
| lib/utils.js | Removes deprecated Buffer allocation compatibility code; re-exports Buffer.alloc/allocUnsafe/from. |
| lib/metadata/data-collection.js | Converts DataCollection to an ES class (but still uses legacy inheritance pattern). |
| lib/metadata/table-metadata.js | Converts TableMetadata to an ES class (currently still calls DataCollection.call). |
| lib/metadata/materialized-view.js | Converts MaterializedView to an ES class (currently still calls DataCollection.call). |
| lib/metadata/schema-index.js | Converts Index to an ES class and moves helpers to static/instance methods. |
| lib/metadata/schema-function.js | Converts SchemaFunction to an ES class. |
| lib/metadata/aggregate.js | Converts Aggregate to an ES class. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
adespawn
force-pushed
the
drop-more-deprecated
branch
from
a812258 to
df754a8
Compare
Draft
nikagra
previously approved these changes
Mar 12, 2026
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.
adespawn
force-pushed
the
drop-more-deprecated
branch
from
5f0208c to
43a0c9c
Compare
Contributor
Author
|
Updated all the comments. Need the approve again, as the last one becomes stale when any changes are introduced. |
Contributor
Author
|
@nikagra Could you approve if you agree with the changes, or let me know if you want more time for the review? |
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
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.
This is a combination of the following minor changes:
package.json(for some reason this bug stayed there for a looong time now)Metadataclass #261)