-
Notifications
You must be signed in to change notification settings - Fork 139
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
Proposed Changes to use the Vercel Build Output for Routing #129
Comments
@dario-piotrowicz just tagging you to see if you had any thoughts about the implementation I have detailed above. For people who have already read the above, note that I have made a minor modification to the proposed implementation. Instead of using arrays for paths extracted during build time like we currently do, I have changed it to be a map of path names -> path data/info (incl. entry points). This makes logical sense when you consider how routing works. More detailed explanation as to why I made the change.Requests come in and we take their pathname. At first, a request pathname for a dynamic route is in the form of When considering that the routing phases intentionally check these non-dynamic and dynamic routes separately, per the Next.js docs, it makes logical sense to opt to use a map that maps pathname -> path data/info. I imagine this would also improve performance as we are no longer looping through an array checking for regex matches in each item. |
Hi @james-elicx I think it looks amazing 🤩, thanks a lot and thanks for pinging me (and thanks for opening an issue first I definitely think it was the right call 🙂), I did read this yesterday, sorry for not replying, one of our team members has been off on holiday and I wanted to check with the whole team on Monday before committing to anything (I think it should be alright but since what you're suggesting is a big operation I would really want to check what everyone on the team thinks first). I'll let you know when I've checked with them 🙂 As for me, first of all removing the middleware manifest is something that we've discussed and that definitely need to do and I've already put comments about that in my refactoring PR so that would really really be a great improvement! Regarding the rest, it seems totally valid, to get some of the finer details I would have to see some code I think but generally it does look very very sound (thank you for your hard work and looking into it! 🙏). I've just got a couple of minor questions:
One concern that I have is that this sounds like a single big bang rewrite? I feel like that wouldn't be the best and that a more gradual approach would be better (so that we can keep up with the various changes and decisions which would be much more difficult to deal with in a huge single PR). Do you think this could be split across multiple steps?
I would greatly prefer this approach as I think it would better for us so that we can have knowledge of the progress and keep familiarity with the code and also better for you since it would have a smaller feedback loop, avoiding the possibility of costly refactoring later down the line and similar issues. I think that such gradual approach would be so much better, but let me know what you think, we don't have to take such approach if it would introduce too much overhead or if the phases can't be implemented only partially |
Apologies in advance for the amount of text in this reply 😅
That is perfectly understandable @dario-piotrowicz , thank you for letting me know.
I do agree that some of the more specific details would be clearer with code to demonstrate it. Trying to piece together exactly how it should flow for production was a bit challenging as it is never detailed anywhere in the documentation - you have to piece it together yourself from the available information and analysing different configs. I also made efforts to seek clarification from Vercel on one of their build output discussion threads last week, but haven't received any responses sadly.
When I write "no handler phase," I am referring to the routes at the start of the config file that are not preceded by any handle definition. When I write returning a response, I am referring to the response created with the properties (headers/status) defined in the matched route in the config. (if the wall of text incoming... So, there are a few optional properties for routes in the config file that alter how things are handled. These are So, if I believe that I believe that
Well, the problem with that is that I don't know how Vercel's platform deals with the config that you upload to it for routing in production. They might transform it when they consume it, or do something else. These proposed changes are based on my understanding of how routing works based on what I have deduced through reading available docs and experimenting - it is not based on how Vercel actually does it in their platform's source code because that is not public. So, any PRs would end up declined as their platform and the build output function how they designed it to work, not how we imagine it to work (this makes sense in my head, not sure if I wrote it down very well). Anyway, the above is laid out in the order that they will logically (to me, anyway) be first consumed. This was to make it easier to understand how the process would work. I am not planning on transforming the config into an alternative order to accommodate my beliefs as that runs the risk of breaking things. Instead, though, I am thinking about grouping each phase's routes into a map so I could do, say, e.g.
This is a good point and I agree. It is certainly possible to do the build time and runtime stuff in separate PRs, and I will happily do so. When it comes to the new routing system with the phases though, I honestly think that it would create more problems trying to split each phase into its own PR than it would be to tackle it all in one PR. This is primarily because almost all of them are interconnected or depend on each other in some way. My intention is to build the runtime routing system in its own file(s), so that it will be as simple as just changing the code in the worker template to call a new function instead of the current process - IMO, the actual switch/turning on can and should be done in a final, separate PR. This also means that it shouldn't be affected by other changes to the codebase. I imagine this will help alleviate the concerns about creating blockers. I shall wait to hear back from you after you speak to the team and for your refactor PR to be merged before I start making any PRs. |
Amazing, thanks so very much again for all the hard work and all the info, your clarification did help, thanks! ❤️ Regardings PRs then is sounds like you're suggesting something like:
Regarding alleviating concerns about creating blockers and conflicts, yeah isolated files would help there, if beneficial we could also put comments or something to indicate what parts of the existing system you'd be replacing 🙂 Anyways thanks a lot for the patience, I'll check with the team and come back to you asap 🙂 |
Yep! That's exactly what I was thinking. IMO, it's the best way to approach this.
Yeah, it might be worth adding some notes to say which parts will later be replaced when doing the initial PR. |
@james-elicx we've discussed this on the team and we'd be grateful if you would go ahead with the refactoring 🙂 😃 |
Hello, back again with another lengthy comment (my apologies) about some solutions to what appears to be a couple of issues that I would appreciate some feedback on. More than happy to jump on a voice call to discuss approaches. 🙂 I've been doing some more research and experimentation with the build output as I wrap up some of the logic for build time. A couple of problems have cropped up that will need addressing. Below I outline the problems and the proposed solutions. If some parts aren't that clear (I'm writing this at night), I am happy to clarify and explain them. While I'm here, I will add that I intend to open a PR for build time changes either tomorrow or the day after. I am intending to implement the solutions detailed below in that PR, but I would still like to discuss them in case anyone has better ideas. (tagging @dario-piotrowicz as I would appreciate your thoughts)
|
@james-elicx thanks a lot for the ultra detailed explanation 🙂 These are my personal thoughts:
|
I understand why you would suggest this, although I would be a bit apprehensive to do it through the next config. The first reason is that we are trying to migrate to only using the build output, but the second is that all routing decisions are based on the file system, so this would be the only instance where we grab part of the data from outside of the build output. i.e., When we check the entry point for a route, we are checking that the path name matches with a path we extracted from the file system, which includes the base path directory. That said, I will do it with the base path from next.config.js if you would prefer that way - it just feels kind of like an unnecessary usage of something outside the build output.
Yeah, so the idea was that when the
Yeah. The good part about solution one is it involves zero changes to how things function, so we can always change it later to add compatibility for the next config i18n if the decision is later made to support that (and ideally if a better way to handle it is come up with). (btw, Kieul's fix in #132 would not work with the new routing system due to how the build output config is laid out with locales as initially explained above).
Agreed. The recommended implementation in the Next docs and the next-intl Next.js 13 examples should both function okay as they just use middleware and dynamic params. That issue is definitely using the i18n option from the next config in combination with next-intl (the two locales not being built for the edge runtime are an indicator of that).
Very strongly agree with well documenting it. As to the source code, I'll take a look, but no promises there.
Understood. I was planning on just writing some unit tests for the parts I add. |
no yeah you totally have a point, it'd be bad to use stuff outside of the build output, my only reasoning is that if feels much more work and less robust to figure out on our own what the basePath is when it is available plan an simple in a js file.
ah yeah I kind of thought that might be the case (the issue is that the
Awesome thanks! 🙏 as I said if it's too complicated/time consuming don't feel forced by at least add some TODOs so that we can remember to do that later 🙂
Sure whatever you think is best, just saying that we're not concerned of having full unit test coverage and if something is difficult to unit test we can just use the hopefully-soon-to-come e2es tests 🙂👍 |
Well, what he did appears to just manually be adding a redirect from So, his solution would end up doing the redirect that we would automatically do in the new routing system, but the problem is that the later locale rewrites need us to step in and tear up the rewritten destination and interfere so that it can properly resolve a route. Basically, it's still the same problem I described above, he is just handling one of the first automatic steps in the output config. Tbh, I'm kind of surprised it's working for him, because I don't believe the regex matchers we currently use from the middleware manifest support a locale at the start of them, unless the locale is somehow getting stripped from the request URL before being handled. Actually, upon further thinking (you can discard my previous paragraph), I don't think his solution is supporting i18n at all. He is only redirecting the |
Revised middleware solutionI have identified a third case for built middleware 🙄
If a middleware has a This should fix the issue from #130 where it appears to sometimes target the uncompiled version instead of the compiled version. |
Hello, this issue outlines the proposed changes that I would like to make to how
next-on-pages
handles routing.The Vercel build output config contains the different steps taken during the routing process used by Vercel, in a logical format, and was designed around making it easy for frameworks to build and deploy on Vercel. This is advantageous in that we can attempt to mirror how their routing works by using this config.
I previously intended to create a PR once I had a working concept, although I feel it would be more beneficial for me to create an issue outlining how I intend it to function. This is so that these thoughts can be discussed and prevent me from wasting time implementing a section that people may disagree on.
I imagine parts of the proposed system may seem confusing at first glance. I am happy to answer any questions you may have about how I have laid it out.
I would appreciate it if this issue could be assigned to me as I would like to build it out :)
Current System
Build time
We extract the middleware manifest from the Next.js build and use this to find our function files. We then minify the entry points and inject the middleware manifest matches and the entry point into the worker script. Additionally, we inject the Vercel build output config.
Runtime
Upon each request, we currently loop through the Vercel build output config, check requirements such as
method
/has
/missing
/caseSensitive
, and then check if thesrc
value matches. If it does, we add the route to a list of matching routes. If there was nosrc
value at all, we also add that handle to the matching routes list. After all that, we check for middleware matches in that list of matching routes and run any middleware. We then loop through all the injected functions and check for a match there. If one is found, we return the value of calling the entry point.Proposed System
These changes remove the need to use the Next.js build output's middleware manifest and rely solely on the Vercel build output. The routing process during runtime is based on the routing handler phases from the Vercel build output config.
Through analysing the result of building with various rewrite options and consulting the Next.js documentation, I further established that dynamic routes and non-dynamic routes are handled at two different points. Additionally, it became clear that dynamic params are handled in the rewrite phase during runtime through two separate regular expressions that add the params as search params. With respect to middleware, the documentation also detailed that they run between redirects and the first rewrite options.
All the phases listed under runtime are in the order that they occur in the build output's config, except for the
resource
phase. In the outputted config, theresource
phase is written betweenfilesystem
andmiss
. I do not believe it should be run at this point based on what I have read, and the below list reflects the points at which each phase will logically run, mostly matching up with the Vercel config. The reason for this is that theresource
phase handles thefallback
rewrite option. Per the Next.js docs, this rewrite option is applied after all routes+static assets are checked, thus leading me to put it where I believe it would logically take place.Tests should of course be added when any implementations of the new system are written.
Build time
Inject the build output config (
.vercel/output/config.json
) into the worker script.Static assets (
.vercel/output/static
):Functions (
.vercel/output/functions
):Inject the map of path names to path data/info into the worker script.
Runtime
no handler phase
beforeFiles
rewrites.continue
=== false -> return response.filesystem
phase (occurs if path name not found in functions list (non-dynamic) or static paths)afterFiles
rewrites.check
=== true -> check filesystem phase again after therewrite
phase.miss
phase (occurs iffilesystem
phase misses - nosrc
match)check
=== true -> check filesystem phase again after therewrite
phase.rewrite
phase (occurs always)check
=== true earlier:filesystem
phase.src
match -> entermiss
phase.resource
phase.resource
phase (occurs ifrewrite
phase ultimately resulted in filesystem miss)fallback
rewrites.check
=== truefilesystem
phase.src
match -> final result is miss.hit
phase (occurs if a function file path or static path matched)error
phase (occurs if response code was 4xx or 5xx)The text was updated successfully, but these errors were encountered: