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 customize content type parsers for api plugin #10573

Merged
merged 16 commits into from
Jun 5, 2024

Conversation

Josh-Walker-GM
Copy link
Collaborator

Alterations to #10449

@Josh-Walker-GM Josh-Walker-GM added the release:feature This PR introduces a new feature label May 15, 2024
@Josh-Walker-GM Josh-Walker-GM added this to the next-release milestone May 15, 2024
@Josh-Walker-GM Josh-Walker-GM self-assigned this May 15, 2024
@dthyresson dthyresson added the changesets-ok Override the changesets check label May 20, 2024
@@ -54,6 +56,11 @@ export async function redwoodFastifyAPI(
}
}

// Run users custom server configuration function
Copy link
Contributor

Choose a reason for hiding this comment

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

Note @Josh-Walker-GM and I found that the order where custom server config was added matters - if after loading functions, then plugins like fastify complress did not work properly.

@dthyresson dthyresson marked this pull request as ready for review May 20, 2024 20:44
@dthyresson dthyresson self-requested a review May 20, 2024 21:01
Copy link
Contributor

@dthyresson dthyresson left a comment

Choose a reason for hiding this comment

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

@Josh-Walker-GM I confirmed I could compress api requests:

image

But, graphql requests are not.

image

Can we add that -- or should we always compress both for api and graphql servers?

We'd still need this in createServer if the user decide to mount and server more that just the api and graphql, but we do want to let people compress graphql responses.

Also - is this a breaking change? If people use register, they will have to reconfigure their server.

@dthyresson
Copy link
Contributor

@Josh-Walker-GM Should we add:

async function main() {
  const server = await createServer({
    logger,
    async configureApiServer(s) {
      await s.register(import('@fastify/compress'), {
        global: true,
        threshold: 1024,
        encodings: ['deflate', 'gzip'],
      })
    },
    async configureGraphQLServer(s) {
      await s.register(import('@fastify/compress'), {
        global: true,
        threshold: 1024,
        encodings: ['deflate', 'gzip'],
      })
    },
``` ?

In case want graphql function to handle differently?

@Josh-Walker-GM
Copy link
Collaborator Author

We can add configure graphql server too if you want. I suspected we'd want to but just kept it as api here to keep the PR small only address the issue that came up.

I don't know if I'd consider this a breaking change. Using register doesn't work and we haven't changed that behaviour - that was incorrect information on our docs to say it did. What we have done here is add in functionality to allow you do configure the server. I'd treat this is a combo of doc fix and feature addition. I might be looking at it wrong though so please push back if you think differently.

@dthyresson
Copy link
Contributor

@Josh-Walker-GM sounds good I’ll make an issue for GraphQL compression as well

@Josh-Walker-GM Josh-Walker-GM removed the changesets-ok Override the changesets check label May 22, 2024
docs/docs/docker.md Outdated Show resolved Hide resolved
docs/docs/docker.md Outdated Show resolved Hide resolved
docs/docs/docker.md Outdated Show resolved Hide resolved
docs/docs/docker.md Outdated Show resolved Hide resolved
docs/docs/docker.md Outdated Show resolved Hide resolved
@Josh-Walker-GM Josh-Walker-GM enabled auto-merge (squash) June 5, 2024 21:01
@Josh-Walker-GM Josh-Walker-GM added fixture-ok Override the test project fixture check and removed fixture-ok Override the test project fixture check labels Jun 5, 2024
@Josh-Walker-GM Josh-Walker-GM merged commit 5eac796 into main Jun 5, 2024
51 checks passed
@Josh-Walker-GM Josh-Walker-GM deleted the jgmw/serverfile-api-config branch June 5, 2024 21:42
Josh-Walker-GM added a commit that referenced this pull request Jun 6, 2024
Alterations to #10449

---------

Co-authored-by: scott1028 <mic1028002@gmail.com>
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
Co-authored-by: David Thyresson <dthyresson@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature This PR introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants