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

Reduce redundancy in the RequestGraph's Request, Env, and Option nodes #9383

Merged
merged 22 commits into from
Nov 16, 2023

Conversation

gorakong
Copy link
Member

@gorakong gorakong commented Nov 14, 2023

↪️ Pull Request

Continued from #9360. This PR reduces redundancy in the RequestGraph's Request, Env, and Option nodes by getting rid of the value object in each of them, since these can be derived from their IDs. On a large app, this nets us a further ~7.1% reduction in the size of the serialized RequestGraph.

💻 Examples

Request Nodes

Before:

  {
    id: 'f20cf057afed92f9:Main',
    type: 'request',
    value: { id: 'f20cf057afed92f9:Main', type: 'path_request' },
    invalidateReason: 0
  }

After (Request Nodes still maintain a request type, but their subtypes can be distinguished via requestType):

 {
    id: 'f20cf057afed92f9:Main',
    type: 'request',
    requestType: 'path_request',
    invalidateReason: 0
  }

Env Nodes

Before:

  {
    id: 'env:NODE_ENV',
    type: 'env',
    value: { key: 'NODE_ENV', value: 'production' }
  }

After:

 { id: 'env:NODE_ENV', type: 'env', value: 'production' }

Option Nodes

Before:

  {
    id: 'option:shouldBuildLazily',
    type: 'option',
    value: { key: 'shouldBuildLazily', hash: 'false' }
  },

After:

{ id: 'option:shouldBuildLazily', type: 'option', hash: 'false' }

|};

export type RequestType =
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

@@ -16,13 +16,15 @@ describe('RequestTracker', () => {
let tracker = new RequestTracker({farm, options});
await tracker.runRequest({
id: 'abc',
// $FlowFixMe[incompatible-call]
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: It might be better to build a mock typecast so there's no any type created by the suppression, which would mean the test won't break if the types change

Something like:

async function mockRunRequest(
      tracker: RequestTracker,
      request: {...Request, type: Request.type | 'mock_request', ...},
      opts?: ?RunRequestOpts,
    ): Promise<TResult> {
      return tracker.runRequest(request, opts);
    }

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call @JakeLane! Let me know what you think about how I mocked runRequest (wasn't sure if that was the best way)

Copy link
Member Author

Choose a reason for hiding this comment

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

@JakeLane just FYI I ended up changing all the mock_requests to be one of the types in the RequestType union. When I tried mocking the types (53fdf80), the only way I found that appeased Flow was to do it in a way that essentially did the same thing (as above) 😅

@gorakong gorakong merged commit 91c6ac5 into v2 Nov 16, 2023
13 of 15 checks passed
@gorakong gorakong deleted the gkong/reduce-cache branch November 16, 2023 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants