-
Notifications
You must be signed in to change notification settings - Fork 551
fix: remove index signature from base request #2923
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
1271c37
a7bf9d8
5c195a3
c019a25
111861c
4dee096
5bf7c59
220258b
314ba7f
810fc22
2847060
cc7be2a
573b320
c31f168
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 |
---|---|---|
|
@@ -55,6 +55,8 @@ describe('ledger', function () { | |
assert.typeOf(ledgerResponse.result.ledger_index, 'number') | ||
|
||
const ledger = ledgerResponse.result.ledger as Ledger & { | ||
// Add index signature to request to iterate through keys and asset types | ||
[index: string]: unknown | ||
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. Similar to my comments in other test files, I don't see the need for this additional kv pair |
||
accepted: boolean | ||
hash: string | ||
seqNum: string | ||
|
@@ -118,6 +120,8 @@ describe('ledger', function () { | |
assert.typeOf(ledgerResponse.result.ledger_index, 'number') | ||
|
||
const ledger = ledgerResponse.result.ledger as LedgerV1 & { | ||
// Add index signature to request to iterate through keys and asset types | ||
[index: string]: unknown | ||
accepted: boolean | ||
hash: string | ||
seqNum: string | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,11 @@ | ||
import { assert } from 'chai' | ||
import omit from 'lodash/omit' | ||
|
||
import { type ServerInfoRequest, type ServerInfoResponse } from '../../../src' | ||
import { | ||
StateAccountingFinal, | ||
type ServerInfoRequest, | ||
type ServerInfoResponse, | ||
} from '../../../src' | ||
import serverUrl from '../serverUrl' | ||
import { | ||
setupClient, | ||
|
@@ -26,7 +30,9 @@ describe('server_info (rippled)', function () { | |
const request: ServerInfoRequest = { | ||
command: 'server_info', | ||
} | ||
|
||
const response = await testContext.client.request(request) | ||
|
||
const expected: ServerInfoResponse = { | ||
id: 0, | ||
result: { | ||
|
@@ -138,29 +144,24 @@ describe('server_info (rippled)', function () { | |
for (const obj of response.result.info.load?.job_types ?? []) { | ||
assert.equal(typeof obj.job_type, 'string') | ||
} | ||
|
||
// state_accounting | ||
Object.keys(response.result.info.state_accounting).forEach(function ( | ||
key, | ||
) { | ||
assert.equal( | ||
typeof response.result.info.state_accounting[key].duration_us, | ||
'string', | ||
) | ||
assert.equal( | ||
typeof response.result.info.state_accounting[key].transitions, | ||
'string', | ||
) | ||
const state_accounting: StateAccountingFinal & { | ||
[index: string]: unknown | ||
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. Why do we need this catch-all type here? I have found that we can iterate over the keys without using this additional key-value pair. |
||
} = response.result.info.state_accounting | ||
Object.keys(state_accounting).forEach(function (key) { | ||
// @ts-expect-error -- non-iterative type | ||
assert.equal(typeof state_accounting[key].duration_us, 'string') | ||
// @ts-expect-error -- non-iterative type | ||
assert.equal(typeof state_accounting[key].transitions, 'string') | ||
}) | ||
|
||
// validated_ledger | ||
assert.equal(typeof response.result.info.validated_ledger?.hash, 'string') | ||
for (const key of Object.keys( | ||
omit(response.result.info.validated_ledger, 'hash'), | ||
)) { | ||
assert.equal( | ||
typeof response.result.info.validated_ledger?.[key], | ||
'number', | ||
) | ||
const validated_ledger = response.result.info.validated_ledger | ||
assert.equal(typeof validated_ledger?.hash, 'string') | ||
for (const key of Object.keys(omit(validated_ledger, 'hash'))) { | ||
// @ts-expect-error -- non-iterative type | ||
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. This type of @ts-expect-error statements can be removed. |
||
assert.equal(typeof validated_ledger[key], 'number') | ||
} | ||
}, | ||
TIMEOUT, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -135,10 +135,12 @@ describe('server_state', function () { | |
key, | ||
) { | ||
assert.equal( | ||
// @ts-expect-error -- non-iterative type | ||
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. Why do we need all these declarations? |
||
typeof response.result.state.state_accounting[key].duration_us, | ||
'string', | ||
) | ||
assert.equal( | ||
// @ts-expect-error -- non-iterative type | ||
typeof response.result.state.state_accounting[key].transitions, | ||
'string', | ||
) | ||
|
@@ -153,6 +155,7 @@ describe('server_state', function () { | |
omit(response.result.state.validated_ledger, 'hash'), | ||
)) { | ||
assert.equal( | ||
// @ts-expect-error -- non-iterative type | ||
typeof response.result.state.validated_ledger?.[key], | ||
'number', | ||
) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
import { assert } from 'chai' | ||
|
||
import type { BaseRequest } from '../../src/models' | ||
|
||
// Helper type assertion function (only for TS, no runtime impact) | ||
function assertType<T>(value: T): void { | ||
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. Since |
||
// No runtime implementation needed | ||
} | ||
|
||
/** | ||
* Type Utility Verification Testing. | ||
* | ||
* Providing ts verification testing for type utilities. | ||
* Chai is not capable of properly type checking non-runtime assertions but this is the closest to simulate behavior. | ||
* Intention is to prevent index signatures on any type models - breaks Omit<> utilty typing | ||
*/ | ||
describe('BaseRequest', function () { | ||
it(`pick utility`, function () { | ||
// Define a value with the picked type | ||
const picked: Pick<BaseRequest, 'command'> = { command: 'test' } | ||
|
||
// Verify the property exists | ||
assert.property(picked, 'command') | ||
assert.typeOf(picked.command, 'string') | ||
|
||
// Verify TypeScript enforces the picked type | ||
assert.isUndefined((picked as any).id) | ||
}) | ||
|
||
it(`omit utility`, function () { | ||
type RequestMinusCommand = Omit<BaseRequest, 'command'> | ||
const control: BaseRequest = { command: 'control' } | ||
// Define a value with the omitted type | ||
const omitted: RequestMinusCommand = { | ||
id: 1, | ||
api_version: 2, | ||
} | ||
|
||
// Verify command is not present | ||
assert.notProperty(omitted, 'command') | ||
assert(typeof control.command === 'string') | ||
|
||
// Verify id properties remain | ||
assert.property(omitted, 'id') | ||
assert.typeOf(omitted.id, 'number') | ||
|
||
// Verify api_version properties remain | ||
assert.property(omitted, 'api_version') | ||
assert.typeOf(omitted.api_version, 'number') | ||
|
||
type HasCommand = 'command' extends keyof BaseRequest ? true : false | ||
type HasNoCommand = 'command' extends keyof RequestMinusCommand | ||
? false | ||
: true | ||
|
||
assertType<true>({} as HasCommand) | ||
assertType<true>({} as HasNoCommand) | ||
Comment on lines
+56
to
+57
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. I don't understand the intent of these two tests. Are you testing that the empty type 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. Well, without an additional dependency, its kind tough the check correct type assertions that are not caught during runtime. Index signature removes type-checking of foreign keys and is not caught during runtime. Trying to write unit tests that are runtime-based is quite difficult for this scenerio. 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. Hmm, I understand. I see the value of the rest of the tests. I didn't understand the need for these two lines only. |
||
}) | ||
|
||
it(`partial utility`, function () { | ||
type OptionalRequest = Partial<BaseRequest> | ||
|
||
// Verify Partial makes all properties optional | ||
const empty: OptionalRequest = {} | ||
assert.notProperty(empty, 'command') | ||
assert.notProperty(empty, 'id') | ||
assert.notProperty(empty, 'api_version') | ||
|
||
// Verify optional nature at compile-time | ||
// This should compile without error | ||
const minimal: OptionalRequest = {} | ||
assert.isUndefined(minimal.command) | ||
}) | ||
}) |
Uh oh!
There was an error while loading. Please reload this page.