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(server): dedupe api server code, make host configurable #9948

Merged
merged 26 commits into from
Feb 1, 2024

Conversation

jtoar
Copy link
Contributor

@jtoar jtoar commented Jan 31, 2024

Follow up to #9884. Does something similar for the api server but I didn't go quite as far.

The crux of this PR is: we duplicated code between @redwoodjs/api-server and @redwoodjs/fastify while we were working on experimental features. This PR brings the changes from @redwoodjs/fastify into @redwoodjs/api-server.

There's still a few types I need help with, and there's some duplicated helper-like function code that I have some ideas around deduplicating, but here's a summary of the changes:

  • Removes ansi-colors

    We only use one colors package across the framework now, chalk. This saves us ~27 kBs. I'm not attached to chalk or anything, but it's used by far more of the framework and we don't need two colors packages.

  • Adds the ability to configure the api/web host and port where possible in many ways

    In working on Docker and with "severful" deploy providers, many of them expect to be able to configure the host and port the server is listening at. Right now in Redwood you really can't configure the host at all. I've added the ability to do this in the ways they'd all expect:

    • Via CLI flag (--apiHost, or just --host depending on the entry point)
    • Via env var (REDWOOD_API_HOST, etc)
    • Via TOML ([api].host)

    And of course if none of these are set, there's our default which is based on NODE_ENV.

    To do this, I had to remove the default for host from @redwoodjs/project-config. The api's value was never hooked up but there may be some code that depended on the web one. I've tested dev and CI tests dev though so I'm fairly certain that things still work out of the box.

  • Changes some file names

    @redwoodjs/api-server had a file called index.ts that wasn't the index (e.g. "main" in the package.json didn't point to it). This file was really the entry point to the package's primary bin, rw-server, so I named it bin.ts.

  • @redwoodjs/api-server uses the package.json exports field

    If there's one explicitly breaking change here, it's this. I know some startups were using this package's default export in a hacky way to create their own server file. I'll have to highlight this in the release notes and emphasize the move to createServer.

    This package also provides the logFormatter bin. You can still run the bin, but I'm not sure if other packages are depending on parts of the code in this directory in an indirect way (I couldn't find any explicit imports).

  • Most fastify plugins don't have to be awaited

    fastify-raw-body is the only plugin that has to be awaited and we were inconsistent with all the others (some packages awaited them; some didn't). I've consistently not awaited those that don't need to be, but I have moved them all above our logic which is important since fastify's loading is graph-based.

  • All code in @redwoodjs/api-server is a fastify plugin

    Most of the plugins were in a kind of decorator with style where you'd pass the object along a series of function calls that'd mutate it. This isn't how fastify prefers to do things; it has a strong encapsulation model built around plugins. Making our functionality plugins goes with instead of against fastify's way of doing things, and we should stop getting "plugin already registered" errors.

Todos after this PR is merged:

  • Update studio which may be affected?

@jtoar jtoar added the release:breaking This PR is a breaking change label Jan 31, 2024
@jtoar jtoar added this to the v7.0.0 milestone Jan 31, 2024
Comment on lines 28 to 31
"./helpers": {
"types": "./dist/helpers.d.ts",
"default": "./dist/helpers"
},
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 have an idea I'd like to float by you @Tobbe instead of going with this pattern again; I did this so that I could share getPort/getHost functionality with the CLI like we did for coerceRootPath

fastify.register(redwoodFastifyAPI, {
redwood: {
...options,
loadUserConfig: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This plugin still loads user config from this entry point, consistent with existing behavior

Comment on lines -312 to -333
export async function redwoodFastifyFunctions(
fastify: FastifyInstance,
opts: RedwoodFastifyAPIOptions
) {
fastify.register(fastifyUrlData)
await fastify.register(fastifyRawBody)

fastify.addContentTypeParser(
['application/x-www-form-urlencoded', 'multipart/form-data'],
{ parseAs: 'string' },
fastify.defaultTextParser
)

fastify.all(`${opts.redwood.apiRootPath}:routeName`, lambdaRequestHandler)
fastify.all(`${opts.redwood.apiRootPath}:routeName/*`, lambdaRequestHandler)

await loadFunctionsFromDist({
fastGlobOptions: {
ignore: ['**/dist/functions/graphql.js'],
},
})
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No more duplication internally anymore either; createServer uses the same plugin apiServerHandler does, just with some fast glob options to ignore the graphql function so it can register it itself

@@ -1,4 +1,3 @@
#! /usr/bin/env node
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 build process adds this now

Comment on lines 14 to 15
// @ts-expect-error TODO(jtoar)
import { coerceRootPath } from '@redwoodjs/fastify-web/helpers'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could use some help with this one; not sure why TS doesn't seem to recognize the subpath imports

Copy link
Member

Choose a reason for hiding this comment

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

Try making it a deep /dist import instead

@@ -70,7 +70,7 @@ export const handler = async ({ side, prisma, dm: dataMigrate }) => {
execaConfig
)
dataMigrate && execa.sync('yarn rw dataMigrate up', execaConfig)
await apiServerHandler({
await apiServerCLIConfig.handler({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deploy target CI will verify these changes for us too

@jtoar jtoar enabled auto-merge (squash) February 1, 2024 13:46
@jtoar jtoar merged commit 887dba9 into main Feb 1, 2024
38 checks passed
@jtoar jtoar deleted the ds-server/dedupe-api-server branch February 1, 2024 14:06
jtoar added a commit that referenced this pull request Feb 1, 2024
Follow up to #9884. Does
something similar for the api server but I didn't go quite as far.

The crux of this PR is: we duplicated code between
`@redwoodjs/api-server` and `@redwoodjs/fastify` while we were working
on experimental features. This PR brings the changes from
`@redwoodjs/fastify` into `@redwoodjs/api-server`.

There's still a few types I need help with, and there's some duplicated
helper-like function code that I have some ideas around deduplicating,
but here's a summary of the changes:

- Removes `ansi-colors`

We only use one colors package across the framework now, `chalk`. This
saves us ~27 kBs. I'm not attached to chalk or anything, but it's used
by far more of the framework and we don't need two colors packages.

- Adds the ability to configure the api/web host and port where possible
in many ways

In working on Docker and with "severful" deploy providers, many of them
expect to be able to configure the host and port the server is listening
at. Right now in Redwood you really can't configure the host at all.
I've added the ability to do this in the ways they'd all expect:

- Via CLI flag (`--apiHost`, or just `--host` depending on the entry
point)
  - Via env var (`REDWOOD_API_HOST`, etc)
  - Via TOML (`[api].host`)

And of course if none of these are set, there's our default which is
based on `NODE_ENV`.

To do this, I had to remove the default for host from
`@redwoodjs/project-config`. The api's value was never hooked up but
there may be some code that depended on the web one. I've tested dev and
CI tests dev though so I'm fairly certain that things still work out of
the box.

- Changes some file names

`@redwoodjs/api-server` had a file called `index.ts` that wasn't the
index (e.g. `"main"` in the package.json didn't point to it). This file
was really the entry point to the package's primary bin, `rw-server`, so
I named it `bin.ts`.

- `@redwoodjs/api-server` uses the package.json exports field

If there's one explicitly breaking change here, it's this. I know some
startups were using this package's default export in a hacky way to
create their own server file. I'll have to highlight this in the release
notes and emphasize the move to `createServer`.

This package also provides the logFormatter bin. You can still run the
bin, but I'm not sure if other packages are depending on parts of the
code in this directory in an indirect way (I couldn't find any explicit
imports).

- Most fastify plugins don't have to be awaited

`fastify-raw-body` is the only plugin that has to be awaited and we were
inconsistent with all the others (some packages awaited them; some
didn't). I've consistently not awaited those that don't need to be, but
I have moved them all above our logic which is important since fastify's
loading is graph-based.

- All code in `@redwoodjs/api-server` is a fastify plugin

Most of the plugins were in a kind of decorator `with` style where you'd
pass the object along a series of function calls that'd mutate it. This
isn't how fastify prefers to do things; it has a strong encapsulation
model built around plugins. Making our functionality plugins goes with
instead of against fastify's way of doing things, and we should stop
getting "plugin already registered" errors.

Todos after this PR is merged:

- [ ] Update studio which may be affected?

---------

Co-authored-by: Tobbe Lundberg <[email protected]>
jtoar added a commit that referenced this pull request Feb 1, 2024
…der (#9949)

Follow up to #9948; missed a
few things at the end there:

- misspelled "description" in a test mock
- the server file and both server handlers take the same options, so I
deduped them
- related, I wasn't passing apiRootPath through to the server file
handler
- I incorrectly accessed `.handler` on `apiServerHandler` in
flightcontrol and render
jtoar added a commit that referenced this pull request Feb 1, 2024
…der (#9949)

Follow up to #9948; missed a
few things at the end there:

- misspelled "description" in a test mock
- the server file and both server handlers take the same options, so I
deduped them
- related, I wasn't passing apiRootPath through to the server file
handler
- I incorrectly accessed `.handler` on `apiServerHandler` in
flightcontrol and render
jtoar added a commit that referenced this pull request Feb 2, 2024
I left these server tests in a not-so-great state after
#9948. Got around to improving
them today with @Josh-Walker-GM. Want to add more, but this covers the
new features I added, mainly the configuration around the web and api
ports and hosts.

I've tried to add this to CI before but they were flakey. I feel like
they're less flakey now and Josh helped me with retry logic. So I think
we could try adding them to CI again and see how it goes.

You can run these tests after building the framework. `cd` into
`tasks/server-tests` and run `yarn vitest run`. The tests also log the
bins they're using and you can run those verbatim.

I split the tests up into separate files mainly because the terminal
becomes unreadable if they're all in the same one.

---------

Co-authored-by: Tobbe Lundberg <[email protected]>
jtoar added a commit that referenced this pull request Feb 4, 2024
I left these server tests in a not-so-great state after
#9948. Got around to improving
them today with @Josh-Walker-GM. Want to add more, but this covers the
new features I added, mainly the configuration around the web and api
ports and hosts.

I've tried to add this to CI before but they were flakey. I feel like
they're less flakey now and Josh helped me with retry logic. So I think
we could try adding them to CI again and see how it goes.

You can run these tests after building the framework. `cd` into
`tasks/server-tests` and run `yarn vitest run`. The tests also log the
bins they're using and you can run those verbatim.

I split the tests up into separate files mainly because the terminal
becomes unreadable if they're all in the same one.

---------

Co-authored-by: Tobbe Lundberg <[email protected]>
jtoar added a commit that referenced this pull request Feb 16, 2024
Running the server file via node means `RWJS_CWD` will never be set so
we can't rely on it to load env files. I seemed to have gotten this
right for the web server in
#9948 but missed it here in the
api server.
jtoar added a commit that referenced this pull request Feb 16, 2024
Running the server file via node means `RWJS_CWD` will never be set so
we can't rely on it to load env files. I seemed to have gotten this
right for the web server in
#9948 but missed it here in the
api server.
jtoar added a commit that referenced this pull request Feb 16, 2024
Ran the k6 tests with @Josh-Walker-GM and we noticed that I forgot to
update one of the entry points in my api server PRs (see
#9948).
jtoar added a commit that referenced this pull request Feb 17, 2024
Ran the k6 tests with @Josh-Walker-GM and we noticed that I forgot to
update one of the entry points in my api server PRs (see
#9948).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:breaking This PR is a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants