-
Notifications
You must be signed in to change notification settings - Fork 540
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
Update eslint config in packages/runtime/runtime-utils to extend "recommended" base config #23956
base: main
Are you sure you want to change the base?
Update eslint config in packages/runtime/runtime-utils to extend "recommended" base config #23956
Conversation
…ommended" base config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
This PR updates the ESLint configuration in packages/runtime/runtime-utils to extend the "recommended" base config and adjusts several files for improved type safety and consistency. Key changes include updating lint rules, standardizing UTF encoding strings from "utf-8" to "utf8", and adding explicit return types as well as minor refactoring throughout the codebase.
Reviewed Changes
File | Description |
---|---|
packages/runtime/runtime-utils/.eslintrc.cjs | Changed ESLint config from a deprecated minimal config to the recommended configuration. |
packages/runtime/runtime-utils/src/dataStoreHelpers.ts | Updated type definitions and error handling logic for improved type safety. |
packages/runtime/runtime-utils/src/handles.ts | Refactored type checking for serialized handles using unknown type. |
packages/runtime/runtime-utils/src/unpackUsedRoutes.ts | Minor refactoring of conditional logic in accumulating child routes. |
packages/common/driver-definitions/src/protocol/storage.ts | Standardized encoding strings from "utf-8" to "utf8". |
packages/runtime/runtime-utils/src/summaryUtils.ts | Added new functions with explicit return types and updated UTF‑8 byte length calculation logic. |
packages/runtime/runtime-utils/src/requestParser.ts | Refactored URL path extraction and added explicit return types. |
packages/runtime/runtime-utils/src/runtimeFactoryHelper.ts | Improved type annotations for factory helper class. |
packages/runtime/runtime-utils/src/objectstorageutils.ts | Updated string slicing and adjusted condition for path existence checking. |
packages/loader/driver-utils/src/blob.ts | Standardized encoding strings in blob constructor. |
packages/loader/driver-utils/src/treeConversions.ts | Standardized encoding strings and minor adjustments in tree conversion. |
Various test files | Updated import paths and added explicit typing for better clarity and Node.js compatibility. |
packages/runtime/runtime-utils/src/utils.ts | Added explicit return type and replaced fromCharCode with fromCodePoint for compact ID encoding. |
Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
packages/runtime/runtime-utils/src/summaryUtils.ts:71
- Verify that the iteration and index decrement logic correctly handles multi-code-unit characters after switching from charCodeAt to codePointAt to ensure the accurate calculation of UTF-8 byte length.
const code = str.codePointAt(i);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still have a few files to go through but leaving some comments for now.
@@ -406,7 +406,7 @@ export class ChannelCollection implements IFluidDataStoreChannel, IDisposable { | |||
const attachMessage = contents as InboundAttachMessage; | |||
// We need to process the GC Data for both local and remote attach messages | |||
const foundGCData = processAttachMessageGCData( | |||
attachMessage.snapshot, | |||
attachMessage.snapshot ?? undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did it complain about this? The snapshot
property matches the types expected by the function parameter...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
processAttachMessageGCData
takes param snapshot
of type ITree
or undefined
but attachMessage.snapshot
is type ITree
or null
function processAttachMessageGCData(snapshot: ITree | undefined, addedGCOutboundRoute: (fromNodeId: string, toPath: string) => void): boolean
export type InboundAttachMessage = Omit<IAttachMessage, "snapshot"> & {
// eslint-disable-next-line @rushstack/no-new-null -- TODO: breaking change; protocol might even explicitly use null
snapshot: IAttachMessage["snapshot"] | null;
};```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I hadn't gotten to the fact that this PR changes the function signature too 😄. The function changes seem fine since nothing inside the function has special casing for snapshot === null
... so yeah, I think this is fine. Double checking, @agarwal-navin and @markfields, any concerns here?
typeof value === "object" && | ||
value !== null && | ||
"type" in value && | ||
value.type === "__fluid_handle__"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit torn here. Not sure that this is better than (value as { type: string | undefined }).type === "__fluid_handle__"
, because we're introducing runtime checks which don't change the function's behavior but add a bit of runtime cost. Maybe @jason-ha has thoughts?
Co-authored-by: Alex Villarreal <[email protected]>
Co-authored-by: Alex Villarreal <[email protected]>
Co-authored-by: Alex Villarreal <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly docs and similar. I'd like to hear back from the summarization crew before approving :)
@@ -455,15 +479,16 @@ export function processAttachMessageGCData( | |||
} | |||
|
|||
assert( | |||
// eslint-disable-next-line unicorn/text-encoding-identifier-case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the one you mentioned where DDS use "utf-8" explicitly, right? If so let's add that to the all the disables in this file, since this is not a lint rule we'll be actively trying to remove, it'll probably stay for a while and we'll want to know why.
Co-authored-by: Alex Villarreal <[email protected]>
Co-authored-by: Alex Villarreal <[email protected]>
Co-authored-by: Alex Villarreal <[email protected]>
Co-authored-by: Alex Villarreal <[email protected]>
Co-authored-by: Alex Villarreal <[email protected]>
Co-authored-by: Alex Villarreal <[email protected]>
Co-authored-by: Alex Villarreal <[email protected]>
Co-authored-by: Alex Villarreal <[email protected]>
Co-authored-by: Alex Villarreal <[email protected]>
Co-authored-by: Alex Villarreal <[email protected]>
🔗 Found some broken links! 💔 Run a link check locally to find them. See linkcheck output
|
The package currently uses our "minimal" config. This PR updates it to use "recommended" and fix the resulting linter violations.
AB#3022