Skip to content
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

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
c773ae2
Update eslint config in packages/runtime/runtime-utils to extend "rec…
RishhiB Feb 28, 2025
d9cf56c
Merge branch 'main' into UpdateESlintToRecommendedRuntimeUtils
RishhiB Feb 28, 2025
46dad36
breaking types
RishhiB Feb 28, 2025
e2caaa5
other package changes
RishhiB Feb 28, 2025
e4453e0
undo utf8 changes
RishhiB Mar 9, 2025
dfb602d
undo more utf-8 changes
RishhiB Mar 9, 2025
dc658b6
Merge branch 'main' into UpdateESlintToRecommendedRuntimeUtils
RishhiB Mar 9, 2025
1066dee
Update packages/runtime/runtime-utils/src/dataStoreHelpers.ts
RishhiB Mar 11, 2025
950f1df
Update packages/runtime/runtime-utils/src/test/handles.spec.ts
RishhiB Mar 11, 2025
d8e3a33
Update packages/runtime/runtime-utils/src/test/handles.spec.ts
RishhiB Mar 11, 2025
2e4aca6
Merge branch 'main' into UpdateESlintToRecommendedRuntimeUtils
RishhiB Mar 11, 2025
85e31a7
helper function
RishhiB Mar 11, 2025
270a49b
format
RishhiB Mar 11, 2025
61c4bfe
Merge branch 'main' into UpdateESlintToRecommendedRuntimeUtils
RishhiB Mar 11, 2025
65bba63
Update packages/runtime/runtime-utils/src/requestParser.ts
RishhiB Mar 11, 2025
5bedcb9
Update packages/runtime/runtime-utils/src/summaryUtils.ts
RishhiB Mar 11, 2025
741db61
Update packages/runtime/runtime-utils/src/summaryUtils.ts
RishhiB Mar 11, 2025
cae0dff
Update packages/runtime/runtime-utils/src/dataStoreHelpers.ts
RishhiB Mar 11, 2025
a15e9bc
Update packages/runtime/runtime-utils/src/dataStoreHelpers.ts
RishhiB Mar 11, 2025
674195c
Update packages/runtime/runtime-utils/src/dataStoreHelpers.ts
RishhiB Mar 11, 2025
7a49240
Update packages/runtime/runtime-utils/src/dataStoreHelpers.ts
RishhiB Mar 11, 2025
2882a42
Update packages/runtime/runtime-utils/src/dataStoreHelpers.ts
RishhiB Mar 11, 2025
90ae168
Update packages/runtime/runtime-utils/src/dataStoreHelpers.ts
RishhiB Mar 11, 2025
d92d273
Update packages/runtime/runtime-utils/src/dataStoreHelpers.ts
RishhiB Mar 11, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,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,
Copy link
Contributor

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...

Copy link
Contributor Author

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;
};```

Copy link
Contributor

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?

(nodeId, toPath) => {
// nodeId is the relative path under the node being attached. Always starts with "/", but no trailing "/" after an id
const fromPath = `/${attachMessage.id}${nodeId === "/" ? "" : nodeId}`;
Expand Down
5 changes: 1 addition & 4 deletions packages/runtime/runtime-utils/.eslintrc.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,7 @@
*/

module.exports = {
extends: [
require.resolve("@fluidframework/eslint-config-fluid/minimal-deprecated"),
"prettier",
],
extends: [require.resolve("@fluidframework/eslint-config-fluid/recommended"), "prettier"],
parserOptions: {
project: ["./tsconfig.json", "./src/test/tsconfig.json"],
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ export function compareFluidHandles(a: IFluidHandle, b: IFluidHandle): boolean;
// @alpha
export function convertToSummaryTreeWithStats(snapshot: ITree, fullTree?: boolean): ISummaryTreeWithStats;

// @alpha (undocumented)
// @alpha
export const create404Response: (request: IRequest) => IResponse;

// @alpha
Expand Down
73 changes: 57 additions & 16 deletions packages/runtime/runtime-utils/src/dataStoreHelpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,45 +17,67 @@ interface IResponseException extends Error {
message: string;
code: number;
stack?: string;
underlyingResponseHeaders?: { [key: string]: any };
underlyingResponseHeaders?: { [key: string]: unknown };
}

/**
* Type guard for determining if an error is an IResponseException
* @internal
*/
export function exceptionToResponse(err: any): IResponse {
function isResponseException(err: unknown): err is IResponseException {
return (
err !== null &&
typeof err === "object" &&
"errorFromRequestFluidObject" in err &&
(err as { errorFromRequestFluidObject: unknown }).errorFromRequestFluidObject === true
);
}

/**
* Converts an error object into an {@link @fluidframework/core-interfaces#IResponse}
* @internal
*/
export function exceptionToResponse(err: unknown): IResponse {
const status = 500;
if (err !== null && typeof err === "object" && err.errorFromRequestFluidObject === true) {
const responseErr: IResponseException = err;
if (isResponseException(err)) {
return {
mimeType: "text/plain",
status: responseErr.code,
value: responseErr.message,
status: err.code,
value: err.message,
get stack() {
return responseErr.stack;
return err.stack;
},
headers: responseErr.underlyingResponseHeaders,
headers: err.underlyingResponseHeaders,
};
}

// Capture error objects, not stack itself, as stack retrieval is very expensive operation, so we delay it
// Capture error objects, not stack itself, as stack retrieval is very expensive operation
const errWithStack = generateErrorWithStack();

return {
mimeType: "text/plain",
status,
value: `${err}`,
get stack() {
return (err?.stack as string | undefined) ?? errWithStack.stack;
// Use type assertion after checking if err is an object with stack
return (
(typeof err === "object" && err !== null && "stack" in err
? (err.stack as string | undefined)
: undefined) ?? errWithStack.stack
);
},
};
}

/**
* Converts an IResponse object back into an Error object that can be thrown
* @param response - The IResponse object to convert
* @param request - The original IRequest object
* @returns An Error object with additional properties from the response
* @internal
*/
export function responseToException(response: IResponse, request: IRequest): Error {
const message = response.value;
const message = response.value as string;
const errWithStack = generateErrorWithStack();
const responseErr: Error & IResponseException = {
errorFromRequestFluidObject: true,
Expand All @@ -72,20 +94,29 @@ export function responseToException(response: IResponse, request: IRequest): Err
}

/**
* Creates a 404 "not found" response for the given request
* @param request - The request that resulted in the 404 response
* @returns An IResponse object with 404 status code
* @legacy
* @alpha
*/
export const create404Response = (request: IRequest) =>
export const create404Response = (request: IRequest): IResponse =>
createResponseError(404, "not found", request);

/**
* Creates an error response with the specified status code and message
* @param status - HTTP status code for the error (must not be 200)
* @param value - Error message or description
* @param request - The request that resulted in this error
* @param headers - Optional headers to include in the response
* @returns An IResponse object representing the error
* @internal
*/
export function createResponseError(
status: number,
value: string,
request: IRequest,
headers?: { [key: string]: any },
headers?: { [key: string]: unknown },
): IResponse {
assert(status !== 200, 0x19b /* "Cannot not create response error on 200 status" */);
// Omit query string which could contain personal data unfit for logging
Expand All @@ -111,6 +142,11 @@ export function createResponseError(
export type Factory = IFluidDataStoreFactory & Partial<IProvideFluidDataStoreRegistry>;

/**
* Creates a combined IFluidDataStoreFactory and IFluidDataStoreRegistry implementation
* from a factory type and implementation
* @param type - The unique identifier for this data store factory
* @param factory - The factory implementation or promise that resolves to one
* @returns A combined factory and registry implementation
* @internal
*/
export function createDataStoreFactory(
Expand All @@ -125,8 +161,13 @@ export function createDataStoreFactory(
get IFluidDataStoreRegistry() {
return this;
},
instantiateDataStore: async (context, existing) =>
(await factory).instantiateDataStore(context, existing),
get: async (name: string) => (await factory).IFluidDataStoreRegistry?.get(name),
instantiateDataStore: async (context, existing) => {
const resolvedFactory = await factory;
return resolvedFactory.instantiateDataStore(context, existing);
},
get: async (name: string) => {
const resolvedFactory = await factory;
return resolvedFactory.IFluidDataStoreRegistry?.get(name);
},
};
}
7 changes: 5 additions & 2 deletions packages/runtime/runtime-utils/src/handles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,11 @@ export interface ISerializedHandle {
* Is the input object a @see ISerializedHandle?
* @internal
*/
export const isSerializedHandle = (value: any): value is ISerializedHandle =>
value?.type === "__fluid_handle__";
export const isSerializedHandle = (value: unknown): value is ISerializedHandle =>
typeof value === "object" &&
value !== null &&
"type" in value &&
value.type === "__fluid_handle__";
Comment on lines +27 to +30
Copy link
Contributor

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?


/**
* Setting to opt into compatibility with handles from before {@link fluidHandleSymbol} existed (Fluid Framework client 2.0.0-rc.3.0.0 and earlier).
Expand Down
16 changes: 12 additions & 4 deletions packages/runtime/runtime-utils/src/objectstorageutils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,18 @@
import { ITree } from "@fluidframework/driver-definitions/internal";

/**
* Normalizes a storage path by removing leading and trailing slashes and splitting into parts
* @param path - The storage path to normalize (e.g. "/foo/bar/")
* @returns Array of path segments (e.g. ["foo", "bar"])
* @internal
*/
export function getNormalizedObjectStoragePathParts(path: string) {
export function getNormalizedObjectStoragePathParts(path: string): string[] {
let normalizePath = path;
if (normalizePath.startsWith("/")) {
normalizePath = normalizePath.substr(1);
normalizePath = normalizePath.slice(1);
}
if (normalizePath.endsWith("/")) {
normalizePath = normalizePath.substr(0, normalizePath.length - 1);
normalizePath = normalizePath.slice(0, -1);
}
if (normalizePath.length > 0) {
return normalizePath.split("/");
Expand All @@ -23,6 +26,11 @@ export function getNormalizedObjectStoragePathParts(path: string) {
}

/**
* Lists all blobs at the specified path in the given tree
* @param inputTree - The tree to search within
* @param path - The path to search at (e.g. "foo/bar")
* @returns Promise that resolves to an array of blob names at that path
* @throws Error if the path does not exist in the tree
* @internal
*/
export async function listBlobsAtTreePath(
Expand All @@ -42,7 +50,7 @@ export async function listBlobsAtTreePath(
// so we must redundantly determine that the entry's type is "Tree"
tree = treeEntry?.type === "Tree" ? treeEntry.value : undefined;
}
if (tree?.entries === undefined || pathParts.length !== 0) {
if (tree?.entries === undefined || pathParts.length > 0) {
throw new Error("path does not exist");
}
return tree.entries.filter((e) => e.type === "Blob").map((e) => e.path);
Expand Down
29 changes: 17 additions & 12 deletions packages/runtime/runtime-utils/src/requestParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,21 +17,25 @@ export class RequestParser implements IRequest {
*/
public static getPathParts(url: string): readonly string[] {
const queryStartIndex = url.indexOf("?");
return url
.substring(0, queryStartIndex < 0 ? url.length : queryStartIndex)
.split("/")
.reduce<string[]>((pv, cv) => {
if (cv !== undefined && cv.length > 0) {
pv.push(decodeURIComponent(cv));
}
return pv;
}, []);
const pathParts: string[] = [];
const urlPath = url.slice(
0,
Math.max(0, queryStartIndex < 0 ? url.length : queryStartIndex),
);

for (const part of urlPath.split("/")) {
if (part !== undefined && part.length > 0) {
pathParts.push(decodeURIComponent(part));
}
}

return pathParts;
}

private requestPathParts: readonly string[] | undefined;
public readonly query: string;

public static create(request: Readonly<IRequest>) {
public static create(request: Readonly<IRequest>): RequestParser {
// Perf optimizations.
if (request instanceof RequestParser) {
return request;
Expand All @@ -41,7 +45,8 @@ export class RequestParser implements IRequest {

protected constructor(private readonly request: Readonly<IRequest>) {
const queryStartIndex = this.request.url.indexOf("?");
this.query = queryStartIndex >= 0 ? this.request.url.substring(queryStartIndex) : "";
this.query =
queryStartIndex >= 0 ? this.request.url.slice(Math.max(0, queryStartIndex)) : "";
if (request.headers !== undefined) {
this.headers = request.headers;
}
Expand All @@ -67,7 +72,7 @@ export class RequestParser implements IRequest {
* Returns true if it's a terminating path, i.e. no more elements after `elements` entries and empty query.
* @param elements - number of elements in path
*/
public isLeaf(elements: number) {
public isLeaf(elements: number): boolean {
return this.query === "" && this.pathParts.length === elements;
}

Expand Down
2 changes: 1 addition & 1 deletion packages/runtime/runtime-utils/src/runtimeFactoryHelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { IContainerRuntime } from "@fluidframework/container-runtime-definitions
* @alpha
*/
export abstract class RuntimeFactoryHelper<T = IContainerRuntime> implements IRuntimeFactory {
public get IRuntimeFactory() {
public get IRuntimeFactory(): this {
return this;
}

Expand Down
Loading
Loading