馃帀 feat v0.1 #2
馃帀 feat v0.1 #2
Conversation
|
Great work @jkcs! I'll review this in a few moments. |
|
@ohsayan Thank you for the help you provided on Discord. After testing and fixing it, everything is working perfectly now. As for the suggestion you made about refactoring it into bl, can we postpone it for now? (I have less free time available currently.) |
|
Just a side note, there are some errors and omissions in the content of the "Networking" section at https://docs.skytable.io/protocol/networking. It seems that Skytable also has a bug regarding the return of integer types. Once I find the steps to reproduce it, I will raise an issue. |
| export class Query { | ||
| private query_buffer: Uint8Array; | ||
| private param_buffer: Uint8Array; | ||
| private param_cnt: number; | ||
|
|
||
| /** | ||
| * Create a new query with the base query set. You can now add (any) additional parameters. | ||
| * | ||
| * @param query The base query | ||
| */ | ||
| constructor(query: string) { | ||
| this.query_buffer = new Uint8Array(Buffer.from(query, 'utf-8')); | ||
| this.param_buffer = new Uint8Array(); |
There was a problem hiding this comment.
We need a Query object! This will allow adding multiple parameters, say in a loop for example.
src/config.ts
Outdated
| return createSkytable(connect); | ||
| } | ||
|
|
||
| async connectTSL(options: ConnectionTLSOptions) { |
There was a problem hiding this comment.
Looks like this is a typo. It should be TLS
src/protocol.ts
Outdated
| return [PARAMS_TYPE.BOOLEAN, Number(param) === 1 ? '\x01' : 0].join( | ||
| '', | ||
| ); | ||
| default: |
There was a problem hiding this comment.
Is this safe to do? If there is a custom type we can't assume it to be a binary blob
src/skytable.ts
Outdated
|
|
||
| export type QueryResult = Column | Row | Rows; | ||
|
|
||
| export function createSkytable(connection: Socket | TLSSocket) { |
There was a problem hiding this comment.
If this function is exported, I think we'll want to call it something like createConnection or connectDB
|
@ohsayan The points you mentioned have all been updated |
|
@ohsayan The I think it's unnecessary for us to refactor the performance improvements into |
There was a problem hiding this comment.
I attempted to import the package to test with this package.json:
{
"name": "jsclient-testbench",
"version": "1.0.0",
"main": "dist/index.ts",
"author": "Sayan Nandan <nandansayan@outlook.com>",
"license": "Apache-2.0",
"devDependencies": {
"typescript": "^5.3.3"
},
"dependencies": {
"skytable": "github:jkcs/client-nodejs#feat/v0.1"
},
"scripts": {
"build": "tsc",
"start": "tsc && node dist/index.js"
}
}and with contents in src/index.ts:
import {Config} from 'skytable';
const cfg = new Config("root", "pass");Are the exports set up correctly to be accessed like this? If not, what is the export structure. An integration test like example would really help!
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
@jkcs thanks for your comment. We already have the publish script set up at publish-npm.yml so don't worry!
I completely forgot about the fact that with GH as the source it won't build.
|
|
Fixes skytable/skytable#324
/claim skytable/skytable#324
TODO LIST