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

feat: add storeLog in Config #21

Merged
merged 18 commits into from
Aug 23, 2024
Merged

feat: add storeLog in Config #21

merged 18 commits into from
Aug 23, 2024

Conversation

nsrCodes
Copy link
Contributor

@nsrCodes nsrCodes commented Aug 8, 2024

will be an optional config argument that expects a function which handles the following log object

export interface Log {
mockId: string;
HarEntry: Partial<Entry>;
}

builds on top of #4 (now stale). Aims at getting the feature released.

CodHeK and others added 9 commits October 31, 2023 23:55
- move logSink inside the configFetcher
- rename configFetcher to config
- rename the internal config in class MockServer to mockConfig
- make config the first argument for the MockServer class
- followed by arguments that have default value (port then prefix)
import { Log } from "../types";

class ILogSink {
store = async (log: Log): Promise<void> => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let rename this to sendLog

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be better to address this one's these changes are merged here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed it in backend implementation here

return res.status(mockResponse.statusCode).set(mockResponse.headers).end(mockResponse.body);
console.debug("[Debug] Final Mock Response", mockResponse);

res.locals.metadata = mockResponse.metadata;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets namespace metadata -> rq_metadata for scoping Requestly variables

src/core/server.ts Show resolved Hide resolved
app: Express

constructor (port: number = 3000, configFetcher: IConfigFetcher, pathPrefix: string = "") {
constructor (port: number = 3000, configFetcher: IConfigFetcher, logSink: ILogSink, pathPrefix: string = "") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logSink should be optional. If not given, should default to empty Sink

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The api of the constructor has been updated in the other PR

response: buildHarResponse(res, { body: responseBody }),
}

storageService.storeLog({ mockId: res.locals.metadata.mockId, HarEntry, })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Faced a crash here while running locally

/Users/sahilgupta/Documents/dev/requestly/requestly-mock-server/src/middlewares/har.ts:28
        storageService.storeLog({ mockId: res.locals.metadata.mockId, HarEntry, })
                                                              ^
TypeError: Cannot read properties of undefined (reading 'mockId')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I faced the same error once. Probably because of the order in which we are applying the metadata in core/server.ts (but still the error shouldn't occur).

It suprisingly went away when I restarted the server and tested with the backend

- convert IConfig from class to interface
- save src and sink in storageService instead of complete IConfig
@nsrCodes nsrCodes added the wip label Aug 21, 2024
@nsrCodes nsrCodes requested a review from wrongsahil August 21, 2024 13:33
Comment on lines 17 to 18
mockConfig: MockServerConfig;
config: IConfig;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These both can be merged into 1 as both are same.

mockServerConfig: MockServerConfig

Also IConfig is incorrect name as prefix I is used for marking abstract classes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be converted to this
constructor (config: MockServerConfig)


class StorageService {
configFetcher ?: IConfigFetcher|null = null;
src: ISource | null = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src -> source

- combine config and mockServerConfig into serverOptions of MockServerClass (handle types accordingly)
- remove the I prefix from storage config
@wrongsahil wrongsahil self-requested a review August 22, 2024 15:47
}

/* To make the constructor options optional except for storageConfig */
type MockServerConstructorOptions = Pick<MockServerOptions, 'storageConfig'> & Partial<MockServerOptions>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can't, we need port and pathprefix as optional there

@nsrCodes nsrCodes merged commit e15c756 into master Aug 23, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants