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

Add GET support to RPC specification #3891

Merged
merged 38 commits into from
Oct 29, 2022
Merged

Add GET support to RPC specification #3891

merged 38 commits into from
Oct 29, 2022

Conversation

siddhsuresh
Copy link
Member

@siddhsuresh siddhsuresh commented Oct 9, 2022

Closes: #2162

What are the changes and their implications?

  • Allow opt-in configuration to use GET requests than POST for query files [ mutations even with this config will only use POST requests]

Example Usage

export const config: ResolverConfig = {
  httpMethod: "GET",
}

But, there is no practical limit for URL size for the latest browsers. So, should the warning be removed?

  • Update the rpc tests to accept GET requests
  • Remove updates to toolkit app - currently present to test

TODO:

  • Replace esbuild usage with jscodeshift to get the config from the files

  • try shifting to swc from recast to improve performance

  • Remove caching layer as it is not needed anymore

Output

(Where GET request has been set for getProject and the default POST request set for getProjects

image

image

@changeset-bot
Copy link

changeset-bot bot commented Oct 9, 2022

🦋 Changeset detected

Latest commit: 5d4e3af

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
blitz Patch
@blitzjs/rpc Patch
@blitzjs/auth Patch
@blitzjs/next Patch
@blitzjs/codemod Patch
@blitzjs/config Patch
@blitzjs/generator Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@siddhsuresh siddhsuresh marked this pull request as ready for review October 9, 2022 11:37
@siddhsuresh siddhsuresh self-assigned this Oct 10, 2022
@siddhsuresh
Copy link
Member Author

siddhsuresh commented Oct 11, 2022

@dillonraphael @beerose what is the recommended way to get the config file from the resolvers?
The ways I have tried till now:

  • Use esbuild and require the resolver file directly and then get the named export config. This method exists till the 4d70c40 commit.
  • Use jscodeshift and recast to use AST to find the named export config by reading the file.

I switched to the second method as it was able to handle re-runs faster when changing the config.httpMethod value in the resolvers on pnpm dev, which I think is due to the for loop in

for (let resolverFilePath of resolvers) {

and am I missing an easier method to get this config?

EDIT [11-10]:

I ran a test to check the time difference between method 1 and method 2 for the

Tested using console.time in the loader-server.ts file just before and after the for loop in windows 11
  console.time("answer time");
  ...
  console.timeEnd("answer time");
  1. esbuild
    image

  2. jscodeshift
    image

Ig this makes method 2 the better choice as it is almost 500 😅 times faster even for low number of resolvers in the toolkit-app. But this is still a 12ms delay from the original, which could become worse in larger projects, to further optimise this will try to use the swc parser to read the config

EDIT 2 [12-10]:

Switched to swc parser and added an LRU caching layer to avoid re-parsing of the file to optimise the reading of the resolver configs

image

@siddhsuresh siddhsuresh requested a review from flybayer October 13, 2022 12:31
packages/blitz-rpc/src/index-server.ts Outdated Show resolved Hide resolved
packages/blitz-rpc/src/index-server.ts Outdated Show resolved Hide resolved
packages/blitz-rpc/src/index-server.ts Outdated Show resolved Hide resolved
packages/blitz-rpc/src/index-server.ts Outdated Show resolved Hide resolved
@siddhsuresh siddhsuresh requested a review from flybayer October 16, 2022 18:47
packages/blitz-rpc/src/index-server.ts Outdated Show resolved Hide resolved
packages/blitz-rpc/src/data-client/rpc.ts Outdated Show resolved Hide resolved
Copy link
Member

@flybayer flybayer left a comment

Choose a reason for hiding this comment

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

awesome, just one small change left!

@siddhsuresh siddhsuresh requested a review from flybayer October 19, 2022 10:57
@itsdillon itsdillon merged commit ceb7db2 into main Oct 29, 2022
@itsdillon itsdillon deleted the add-optional-get-rpc branch October 29, 2022 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] add GET support to rpc specification
4 participants