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(remix-dev): add serverEntryFile config option #4123

Closed
wants to merge 2 commits into from

Conversation

kiliman
Copy link
Collaborator

@kiliman kiliman commented Sep 2, 2022

Currently if a dev wants to extend the configuration of Remix App Server, they
must eject from RAS to the Express adapter. This PR adds a new config option
serverEntryFile that specifies the file that will be used by RAS instead of
its default configuration. If the file is TypeScript, it will be transpiled
automatically.

// remix.config.js
module.exports = {
  serverEntryFile: './server.ts'
}

Running npm run dev will launch Remix App Server and import the server file.

For production builds, the dev should update the start script to specify the
server file for remix-serve. NOTE: If the server file is TypeScript, this must
be transpiled manually during the build process.

"build": "remix build && npm run build:server",
"build:server": "esbuild --format=cjs --platform=node --outfile=build/server.js server.ts",
"start": "remix-serve build build/server.js",

The server file should export the following functions:

// REQUIRED: create the Express app and return it
export function createApp(
  buildPath: string,
  mode = "production",
  publicPath = "/build/",
  assetsBuildDirectory = "public/build/"
): Express

// OPTIONAL: create an HTTP/HTTPS server and return it
function createServer(app: Express, port: number): Server

// OPTIONAL: create a WebSocket server and return it
function createSocketServer(port: number): WebSocket.Server

Here's a sample server file that includes all the exports.

https://github.com/kiliman/remix-ras-server-example/blob/main/server.ts

Currently if a dev wants to extend the configuration of Remix App Server, they
must eject from RAS to the Express adapter. This PR adds a new config option 
`serverEntryFile` that specifies the file that will be used by RAS instead of
its default configuration. If the file is TypeScript, it will be transpiled
automatically.

```js
// remix.config.js
module.exports = {
  serverEntryFile: './server.ts'
}
```

Running `npm run dev` will launch Remix App Server and import the server file.

For production builds, the dev should update the `start` script to specify the
server file for `remix-serve`. NOTE: If the server file is TypeScript, this must
be transpiled manually during the build process.

```json
"build": "remix build && npm run build:server",
"build:server": "esbuild --format=cjs --platform=node --outfile=build/server.js server.ts",
"start": "remix-serve build build/server.js",
```
    
The server file should export the following functions:

```ts
// REQUIRED: create the Express app and return it
export function createApp(
  buildPath: string,
  mode = "production",
  publicPath = "/build/",
  assetsBuildDirectory = "public/build/"
): Express

// OPTIONAL: create an HTTP/HTTPS server and return it
function createServer(app: Express, port: number): Server

// OPTIONAL: create a WebSocket server and return it
function createSocketServer(port: number): WebSocket.Server
```
@changeset-bot
Copy link

changeset-bot bot commented Sep 2, 2022

⚠️ No Changeset found

Latest commit: 45f916d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@kiliman
Copy link
Collaborator Author

kiliman commented Sep 2, 2022

Aside from the readConfig test, I haven't added or modified any tests. I'm not really sure the best approach to testing this as it requires verifying remix dev, remix watch and remix-serve is processing the server file correctly.

Please advise.

@cliffordfajardo
Copy link
Contributor

cliffordfajardo commented Sep 3, 2022

Nice! I liked what you mentioned here in #4107 (comment below)

[with this new change] ...you can specify your own server file for Remix App Server to customize your setup. This way Remix doesn't have to add a bunch of different configuration options. It gives you the flexibility of the Express adapter, but the DX of being able to simply run remix dev.


@remix-run/dev has a config.ts file which expose a server property, which one can use to define a server entry point according to the JSDoc comment right above it

I recall seeing @mcansh using that server property in this examples folder of his `remix-fastify repo a few days ago

@kiliman - is there an overlap with the new serverEntryFile config property and the existing server config property defined here?

@kiliman
Copy link
Collaborator Author

kiliman commented Sep 3, 2022

Yes there is a difference. The server option compiles your server file AND bundles it with your app build (including node_modules) into a single file.

The new option is just an extension of Remix App Server. I suppose I could have added an argument to remix dev instead.

@cliffordfajardo
Copy link
Contributor

cliffordfajardo commented Sep 9, 2022

@kiliman - do you have any recommendations on how to test these changes out for folks who want to give your changes a test run?

  • do you have a patch file for patch-package
  • or if we clone your branch, after cloning your branch how would you recommend to proceed?

I'd love to provide feedback and trial out the API and report my findings back in this thread

@kiliman
Copy link
Collaborator Author

kiliman commented Sep 10, 2022

@cliffordfajardo I created a sample showing how to use a custom server file for RAS. It uses SSL and includes getLoadContext function.

https://github.com/kiliman/remix-ras-server-example

@MichaelDeBoey MichaelDeBoey changed the title feat: Add option to specify server file for Remix App Server feat(remix-dev): add serverEntryFile config option Sep 29, 2022
@jamesarosen
Copy link

It might be nice to show how consumers can use this to get the default @remix-run/serve server plus extra middleware:

// server/index.ts
import { createApp } from '@remix-run/serve'
import express from 'express'
import path from 'path'
import myCustomMiddleware from './myCustomMiddleware'

const app = express()
app.use(myCustomerMiddleware)
app.use(createApp(path.join(process.cwd(), 'build'));
export default app

That way, if folks follow the docs, they'll still be as close to @remix-run/serve as possible. They can always customize further if they want.

@kiliman
Copy link
Collaborator Author

kiliman commented Oct 20, 2022

@jamesarosen That's basically what this PR does. The issue is that the current Remix App Server doesn't expose the Express server, nor provide a way to override it.

This PR provides that extension point.

@jamesarosen
Copy link

That's basically what this PR does

I understood. I wasn't clear, but I was saying the docs should prefer importing the default server and extending it:

import { createApp } from '@remix-run/serve'

app.use(myCustomMiddleware)
app.use(createApp())

over replacing it altogether:

const { createRequestHandler } = require("@remix-run/express");

app.use(compression())
app.disable("x-powered-by")
app.use("/build", express.static("public/build", { immutable: true, maxAge: "1y" }))
app.use(express.static("public", { maxAge: "1h" }))
app.use(morgan("tiny"))
app.all(createRequestHandler())

That way, people who follow the docs will get the benefit of the default createApp with its caching, logging, and any new features the community adds over time.

@kiliman
Copy link
Collaborator Author

kiliman commented Oct 21, 2022

Ah, I think I see. So you're saying that in addition to my PR, make the default createApp an export so it can be used and extended in the server.ts file.

I agree. Sometimes you only want to make a simple change to the default, so may not want to write it from scratch (even if it is boilerplate).

My only concern is that Express app configuration is order dependent, so that my have unintended consequences, especially if the default is a black box.

@jamesarosen
Copy link

make the default createApp an export so it can be used and extended in the server.ts file.
Exactly!

My only concern is that Express app configuration is order dependent, so that my have unintended consequences, especially if the default is a black box.

There are definitely pros and cons to both approaches. I don't think there's a clear winner. I spent a lot of time in the Ember community, which places a lot of value on preferring the framework, but offering a series of escalating escape hatches. In this case, that series would be

  1. use @remix-run/serve. A basic Remix-friendly HTTP server with TypeScript and live-reloading.
  2. serverEntryFile with createApp(). Add features to the basic server. Opt in to updates from the community.
  3. serverEntryFile with custom createRequestHandler(). You still get TypeScript and live-reloading, but opt out of updates from the community.
  4. server (I think; I'm not actually sure why someone would use this if serverEntryFile exists)
  5. replace @remix-run/serve with node myServer.js. Totally custom. No TypeScript or live-reloading unless you build it (e.g. with tsc and nodemon)

@kiliman
Copy link
Collaborator Author

kiliman commented Oct 21, 2022

  1. server is for hosts that don't allow you to run npm i for node_modules. This way you bundle everything and copy a single file to your host. Probably pretty rare considering how many providers there are.

@danditomaso
Copy link

This change is exactly what I needed within Remix, I have lightly modified my express config but am now stuck using nodemon/tsc as described above. Using the framework in this case with an extended server.{t|j}s file would be such an benefit. Thanks for the hard work! Is there a timeline of when this would be merged into the core codebase?

@MichaelDeBoey
Copy link
Member

@kiliman I think we can close this as we now have entryServerFilePath config option since @mcansh's #4600?

@mcansh
Copy link
Collaborator

mcansh commented Mar 9, 2023

I think we can close this as we now have entryServerFilePath config option since @mcansh's #4600?

thats on the resolved config for entry.server - not the user's config

@MichaelDeBoey
Copy link
Member

Oh yeah I missed that it's not in the user's config, but the resolved config 🙈

@kiliman
Copy link
Collaborator Author

kiliman commented Mar 9, 2023

Yeah, I haven't been keeping this updated with the latest, as so many changes were happening with the new dev server.

@MichaelDeBoey
Copy link
Member

Hi @kiliman!

Are you still interested in getting this one merged?

If so, please rebase onto latest dev & resolve conflicts

@MichaelDeBoey MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label May 1, 2023
@kiliman
Copy link
Collaborator Author

kiliman commented May 1, 2023

Hmm.. not sure. I think the new unstable_dev feature obviates the need for some of the items in this PR. I personally don't use it. I'll close it for now. I'll reconsider if somebody wants me to update it to the latest version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed needs-response We need a response from the original author about this issue/PR NEW API package:dev package:serve
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants