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

Allow overriding the Server module with a subclass #1388

Closed
wants to merge 3 commits into from

Conversation

jesseditson
Copy link
Contributor

  • add a new config option, Server - which can be provided a subclass of next/server to modify server functionality (such as routing)
  • add the ability to require('next/server') for server subclasses
  • add an example of usage

background/comments

I have been enjoying most of working with next.js, but found that the lack of a clear way to add parameterized routes was my biggest pain point in getting started and staying productive with the framework.

This PR is an attempt to provide an extension point so that we can customize server behavior without having to toss out the advantages of running via next dev|start. I've provided a patch that only adds a few lines of code, and an example of how one might take advantage of the new functionality to simplify parameterized routing.

To me, this is probably the biggest pain point in next, so even if this PR doesn't land, I'd love to chat about the willingness of the zeit team to explore adding params to the core API, or if extension points like this are the right path forward.

Thanks a bunch for the excellent lib, this is one of the closest projects I've seen to achieving the SSR holy grail.

@rauchg
Copy link
Member

rauchg commented Mar 12, 2017

So, to be clear, what you want to retain is the usage of next, as opposed to node server.js? Can you elaborate on the painpoint a bit more?

Because this looks pretty straightforward to me 👇

https://github.com/zeit/next.js/blob/master/examples/custom-server-express/server.js

@rauchg
Copy link
Member

rauchg commented Mar 12, 2017

Thanks for starting up this important discussion 👌

@jesseditson
Copy link
Contributor Author

Absolutely!

I started this work just using my own server and requiring next as in the custom-server-express example - this seems great for applications that have tightly coupled application servers, but in my case, my application is a FE-only renderer that is powered by a microservice - so having to create a server.js at all was a bit disappointing.

My thinking with the above is that it would be easy to modularize common server patterns and once again return to the bliss of having the server logic being fairly invisible.

The reasoning behind a subclass over an instantiation is mainly around the binaries next-dev and next-start - I like these tools a lot in my workflow, and it seemed a shame to lose the subprocess and error handling you've written in them to be able to use parameters.

My hope was to find a happy medium between writing all the server boilerplate myself and needing to slightly tweak the routing logic. Perhaps I'm missing a simpler way to retain the usefulness and simplicity of running the next binary while still customizing routing logic?

Thank you for the quick response!

@jesseditson
Copy link
Contributor Author

jesseditson commented Mar 12, 2017

I figure this probably comes down to more of a business-level design decision - is next.js intended to be omakase when it comes to a majority of FE use cases, with a tipping point at which the consumer has to recreate the logic behind some of the up-and-runnning toolset, or is it a more general rendering tool that has some amount of customization, but is generally going to need to be coupled with other server logic to be useful in a production environment?

It seems like if the goal is the former, then the right solution would be to just build parameterized routing in either via config or something like https://github.com/jesseditson/fs-router (I obviously prefer the latter, as it's a more easily trackable pattern).

If the goal is the latter, I think adding extension points to next.config.js might be a good approach, especially because of the possibility to do things like:

module.exports = {
  Server: require('next-parameter-server')([ ...config ])
}

(contrived, but in theory could be a clean extension API that is somewhat safeguarded against a config-hell situation)

This would mean a consumer could add logic in a modular way, but still keep their codebase purely presentation focused, and use the same toolset (next dev, next start) all the way in to prod.

@sedubois
Copy link
Contributor

About using custom server vs next start: it's also painful for hot-reloading. With custom server one needs to use nodemon, which leads to figuring out exactly which files should or shouldn't trigger a server reload. Very cumbersome. Moreover, when simultaneously modifying client and server files I ended up repeatedly in a situation where next didn't restart correctly, forcing me to kill the process. Finally, the console messages are totally different between nodemon and next so makes for poor DX.

So +100 for this idea :-)

@rauchg
Copy link
Member

rauchg commented Mar 12, 2017

About using custom server vs next start: it's also painful for hot-reloading. With custom server one needs to use nodemon, which leads to figuring out exactly which files should or shouldn't trigger a server reload.

Yeah, one way we can address this is by transpiling import / export for Node, since it's inevitable there.

I think maybe we can address this issue with next -s and next start -s, supplying a server file that exports a function that creates and returns the next server.

@sedubois
Copy link
Contributor

I think maybe we can address this issue with next -s

That seems like the way to go yes. FYI this hot-reloading issue was reported in #1012.

@jesseditson
Copy link
Contributor Author

I like the approach of next -s - could I suggest also allowing a now.config.js option as well? I love the cleanliness and lack of duplication between next and next dev when it comes to config.

- adds a new config option, `Server` - which can be provided a subclass of next/server to modify server functionality (such as routing)
- adds the ability to `require('next/server')` for server subclasses
@timneutkens timneutkens added this to the 2.1 milestone Mar 15, 2017
@rauchg
Copy link
Member

rauchg commented Mar 15, 2017

@jesseditson to clarify: programmatic usage of next still looks for and understands next.config.js

@jesseditson
Copy link
Contributor Author

hehe maybe I should clarify as well! My hope would be that I'd have a way to define the argument that would be passed via -s inside the next.config.js, so I can run next (no args) with a custom server side router.

I'd like to avoid having to modify my package.json scripts values to use a custom server if possible!

It looks to me like there isn't really a way to programmatically bring up the whole stack - and reading over the bin files it seems to me like the subprocess forking is in the right place there, and that the next entry point is always going to have a slightly different usage pattern and feature set than programmatic usage. My singular goal here is to be able to leverage the entire feature set of next and to add parameterized routing, but there would obviously be other advantages of being able to modify the server routing (and middleware type stuff).

This leads me to feel like the clearest path to being able to get both existing and future conveniences provided by the next binary and parameterized routing is either first-party support for parameters or a hook of some sort (config is fine) for the built in next Server to be augmented.

Hopefully that's helpful in clarifying my goals, I apologize if I'm being a bit verbose!

@jesseditson
Copy link
Contributor Author

Curious: milestone 2.1 has come and gone, should I rebase this against master or will this be addressed differently? I'd be happy to implement an -s option on a separate PR if that's preferable - let me know how I can help get this type of functionality implemented!

@timneutkens timneutkens removed this from the 2.1 milestone Jul 1, 2017
@jesseditson
Copy link
Contributor Author

A project brought me back to this area - what's the status on this?

@jesseditson
Copy link
Contributor Author

Hi! Was using next export for a project, and this once again was a sticking point for me. Here's my latest setup:

  • next project that will be exported to a docs site
  • uses a single page to render all pages, and uses the exportPathMap and as links to create fake custom paths
  • when exported, those "fake" paths become real

The problem is obviously that if I click a link in dev and reload, the site will 404. Needless to say that's not optimal, and the rift between dev and prod is painful for testing.

If this PR's API were available, I could easily load in the route map to the server and have hot-reload and be able to refresh and use plain anchor links locally.

My question for @rauchg (or maybe @arunoda): Is there a long-term plan for fixing #1012, and/or allowing us to use the built-in hot reloading and a custom server or routes? This seems like a huge hole in functionality for me.

It's been a bit over 6 months since this PR was opened, and I still need this functionality. I'd appreciate it if you closed this PR or provided a path to merging it so I can decide if next is right for these types of projects.

@DaveStein
Copy link

@jesseditson Looks like you need to resolve some conflicts and then maybe ask for a review? Just caught the other issues with you and @sedubois : #1012 and #791. I'm also hoping I can just rely on next to restart the express server when changes are made, rather than needing to add nodemon or another dep. Afterall, the beauty of next is that it just works without needing to specify more deps like webpack and whatnot. I'm just starting with Next so not sure you'd want me resolving things in your PR ;)

@jesseditson
Copy link
Contributor Author

@DaveStein hey! This PR is quite stale, and as such will likely take a bit of work to get working again. Given that I haven't heard anything from the core maintainers in a long time, I'm hesitant to throw much time at this since it seems like a dead end.

Like you, I still have this problem (just encountered it again last week actually), and I've been keeping an eye on this to see if there's any movement WRT willingness to allow this type of customization.

To be clear for the Next team: I'd be happy to rebase/refactor this to work with latest as soon as there's any signal from your end that this PR would be considered if I go to that effort, just say the word.

@timneutkens
Copy link
Member

Let's move this discussion into an RFC issue, feel free to create one 🙏 One of the reasons I've kept this PR open for 1,5 years is that it's kind of an interesting idea. There are downsides however, so I do encourage creating an issue for it 🙏

@jesseditson
Copy link
Contributor Author

Thanks @timneutkens! Much appreciate the explanation. I'm not on next daily anymore, but I'm sure a project will bring me back at some point. Until then I'd be happy to participate but am probably too out of date with next to sponsor an RFC personally.
If anyone else on the thread feels like this issue is important enough to them to RFC this I'd be happy to document the use cases that led me to this work! ✌️

@timneutkens
Copy link
Member

👍🙌 No worries, we're thinking about several other implementations too, since serverless support is coming 👍

@lock lock bot locked as resolved and limited conversation to collaborators Dec 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants