refactor(ordered-collection): Updated eslint to use recommended config #23335
Conversation
github-actions
bot
added
area: dds
There was a problem hiding this comment.
Copilot reviewed 5 out of 7 changed files in this pull request and generated no comments.
Files not reviewed (2)
- packages/dds/ordered-collection/src/snapshotableArray.ts: Evaluated as low risk
- packages/dds/ordered-collection/src/consensusQueue.ts: Evaluated as low risk
Comments suppressed due to low confidence (1)
packages/dds/ordered-collection/src/consensusOrderedCollection.ts:416
- The return type of the deserializeValue method is marked as unknown, which might cause issues if the deserialized value is used without proper type checks. Consider updating the return type to a more specific type if possible.
private deserializeValue(content: string, serializer: IFluidSerializer): unknown {
| @@ -108,6 +108,8 @@ export interface IConsensusOrderedCollectionEvents<T> extends ISharedObjectEvent | |||
| * @legacy | |||
| * @alpha | |||
| */ | |||
|
|
|||
There was a problem hiding this comment.
Not sure if this extra blank line would break the connection between the TSDoc block above and the interface?
There was a problem hiding this comment.
I don't believe it does. TSDoc comments are associated with the first valid code that follows them, regardless of blank lines or directives like eslint-disable. You can verify this by hovering over the interface showing the related comment.
There was a problem hiding this comment.
Still probably best to remove the blank line though
| @@ -61,30 +61,31 @@ function createCollectionForReconnection( | |||
| } | |||
|
|
|||
| describe("ConsensusOrderedCollection", () => { | |||
| /* eslint-disable @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-assignment */ | |||
There was a problem hiding this comment.
Did we try to use unknown[] here? It's not public API so it should be a safe change to make.
There was a problem hiding this comment.
We could also make this function generic, assuming tests are doing anything other than equality comparisons with the input/output entries.
| let testCollection: ConsensusOrderedCollection; | ||
|
|
||
| async function removeItem() { | ||
| async function removeItem(): Promise<any> { |
There was a problem hiding this comment.
Let's not introduce new uses of any. Same below.
| async function removeItem(): Promise<any> { | |
| async function removeItem(): Promise<unknown> { |
| @@ -211,6 +213,7 @@ describe("ConsensusOrderedCollection", () => { | |||
| await addItem(obj); | |||
| const result = await removeItem(); | |||
| assert.notStrictEqual(result, obj); | |||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access | |||
There was a problem hiding this comment.
With a change I suggest above, removeItem() would start returning Promise<unknown> and we can address the issue of accessing .x by giving an explicit type to the unknown thing, something like const result: { x: number } = await removeItem();.
| @@ -392,37 +398,37 @@ describe("ConsensusOrderedCollection", () => { | |||
| this.containerRuntimeFactory, | |||
| ); | |||
| } | |||
|
|
|||
| private async addItem(item: any) { | |||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
There was a problem hiding this comment.
Similar to another comment, since this is all private code, let's move to unknown.
There was a problem hiding this comment.
We should be able to remove the eslint disable now :) and same on line 408.
packages/dds/ordered-collection/src/consensusOrderedCollection.ts
Outdated
Show resolved
Hide resolved
packages/dds/ordered-collection/src/consensusOrderedCollection.ts
Outdated
Show resolved
Hide resolved
WayneFerrao
force-pushed
the
update-ord-col
branch
from
085577f to
a184ccd
Compare
| @@ -392,37 +398,37 @@ describe("ConsensusOrderedCollection", () => { | |||
| this.containerRuntimeFactory, | |||
| ); | |||
| } | |||
|
|
|||
| private async addItem(item: any) { | |||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
There was a problem hiding this comment.
We should be able to remove the eslint disable now :) and same on line 408.
| @@ -72,4 +72,5 @@ export const ConsensusQueue = createSharedObjectKind(ConsensusQueueFactory); | |||
| * @legacy | |||
| * @alpha | |||
| */ | |||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
There was a problem hiding this comment.
TODO with work item number?
| @@ -35,6 +35,7 @@ class SnapshotableQueue<T> extends SnapshotableArray<T> implements IOrderedColle | |||
| * @legacy | |||
| * @alpha | |||
| */ | |||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
There was a problem hiding this comment.
TODO with work item number?
| @@ -108,6 +108,7 @@ export interface IConsensusOrderedCollectionEvents<T> extends ISharedObjectEvent | |||
| * @legacy | |||
| * @alpha | |||
| */ | |||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
There was a problem hiding this comment.
TODO with work item number? And below
Description
This PR updates the eslint config for dds/ordered-collection to use the recommended config