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

Proposed Changes to use the Vercel Build Output for Routing #129

Closed
james-elicx opened this issue Mar 22, 2023 · 12 comments · Fixed by #206
Closed

Proposed Changes to use the Vercel Build Output for Routing #129

james-elicx opened this issue Mar 22, 2023 · 12 comments · Fixed by #206
Assignees
Milestone

Comments

@james-elicx
Copy link
Contributor

james-elicx commented Mar 22, 2023

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 the src value matches. If it does, we add the route to a list of matching routes. If there was no src 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, the resource phase is written between filesystem and miss. 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 the resource phase handles the fallback 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):

  • Extract the path names.
  • Add the path names to a map of path names to path info.

Functions (.vercel/output/functions):

  • Extract the path names.
  • Extract the entry points.
  • Minify the entry points.
  • Add the path names to a map of path names to path info (with minfied entry point).

Inject the map of path names to path data/info into the worker script.

Runtime

no handler phase

  • applies headers.
  • applies redirects.
  • applies/processes middleware.
  • applies beforeFiles rewrites.
  • continue === false -> return response.

filesystem phase (occurs if path name not found in functions list (non-dynamic) or static paths)

  • applies afterFiles rewrites.
  • check === true -> check filesystem phase again after the rewrite phase.

miss phase (occurs if filesystem phase misses - no src match)

  • check === true -> check filesystem phase again after the rewrite phase.

rewrite phase (occurs always)

  • applies dynamic params.
  • check pathname is in functions (incl. dynamic) or static paths.
    • miss ->
      • if check === true earlier:
        • enter filesystem phase.
          • no src match -> enter miss phase.
      • else:
        • enter resource phase.

resource phase (occurs if rewrite phase ultimately resulted in filesystem miss)

  • applies fallback rewrites.
  • check === true
    • enter filesystem phase.
      • no src match -> final result is miss.

hit phase (occurs if a function file path or static path matched)

  • call the function's entrypoint.
  • return response with applied headers + status code.

error phase (occurs if response code was 4xx or 5xx)

  • if status matches, rewrite to dest.
  • return response.
@james-elicx
Copy link
Contributor Author

@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 /user/1, instead of /user/[id], as it has not been rewritten yet. Non-dynamic routes and static assets are checked first. Their path names should match the path name in the file path. Later, we enter the rewrite routing phase. Here, paths with dynamic parameters are rewritten using regex from /user/1 to /user/[id]?id=1. This rewritten pathname will now match the file path for the dynamic route, and dynamic routes are checked.

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.

@dario-piotrowicz
Copy link
Member

dario-piotrowicz commented Mar 24, 2023

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:

  • in the no handler phase, what do you mean with? continue === false -> return response what's continue and what response would we be returning?
  • would there be a benefit (or downside) in keeping the resource phase in the same order as Vercel? (if they got it wrong maybe we could look into that and open an issue/PR there?)

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?
like a for example:

  • a PR for applying the build time changes (including runtime tweaks to handle the new data format),
  • followed by one to apply the new structure on runtime and implement core phases
  • and then different PRs for each remaining phase.

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.
(One single time huge PR would also create issues like us being blocked from/scared of touching various parts of the codebase and/or you possible getting constant merge conflicts).

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

@james-elicx
Copy link
Contributor Author

james-elicx commented Mar 25, 2023

Apologies in advance for the amount of text in this reply 😅

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).

That is perfectly understandable @dario-piotrowicz , thank you for letting me know.

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 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.

I've just got a couple of minor questions:

  • in the no handler phase, what do you mean with? continue === false -> return response what's continue and what response would we be returning?

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.

Example image

image

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 hit phase is involved, I am also speaking about the response from calling the matched route's entry point)

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 continue, important, and override. continue is documented as being used to change matching behaviour, and the latter two are not documented from what I can tell. If continue is true, routing continues even when the src regex is matched to the pathname.

So, if continue is not equal to true in the no handler phase, i.e. it is undefined, we can reason that this match is the ultimate final one that should be used to return a response. As an example, In the no handler phase, redirects are handled. In my next.config.js, I have defined a redirect to send people visiting /about to /. This is the only route in the no handler phase that does not have continue: true. As this is a redirect, it would make sense that you now want to exit the routing and instead return the response object created for the headers/status that the redirect defined. Hence, this option does not have continue: true specified on it.

I believe that important is used to imply that the headers specified are more important that any other headers in the response object returned by running the entry point. Why do I think this? Well, the only example of it being used is in routes in the hit phase to set the cache-control header and the x-matched-path response header. The latter is listed as a reserved response header by Vercel, so this leads me to believe that important is telling the routing system that the header takes precedence over others.

I believe that override means that the newly matched route completely replaces the previously matched one. At the start of the no handler phase, there is a redirect that redirects from /.../.../.../ to .../.../.... However, I could have a rewrite rule in my next.config.js that rewrites /x/ to /y/. This route has override: true in the build output's config. One can logically assume that this is so that it takes precedence over the response created from the previous match.

  • would there be a benefit (or downside) in keeping the resource phase in the same order as Vercel? (if they got it wrong maybe we could look into that and open an issue/PR there?)

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. router.get('filesystem') to get everything in the filesystem phase.

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? like a for example:

  • a PR for applying the build time changes (including runtime tweaks to handle the new data format),
  • followed by one to apply the new structure on runtime and implement core phases
  • and then different PRs for each remaining phase.

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. (One single time huge PR would also create issues like us being blocked from/scared of touching various parts of the codebase and/or you possible getting constant merge conflicts).

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

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.

@dario-piotrowicz
Copy link
Member

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:

  • PR for updating the build time logic + small refactoring to handle that in the runtime
  • PR to implement the runtime routes without enabling them
  • PR to switch the runtime routes on and remove all no-longer needed code
    did I got it right? 🙂

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 🙂

@james-elicx
Copy link
Contributor Author

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:

  • PR for updating the build time logic + small refactoring to handle that in the runtime
  • PR to implement the runtime routes without enabling them
  • PR to switch the runtime routes on and remove all no-longer needed code
    did I got it right? 🙂

Yep! That's exactly what I was thinking. IMO, it's the best way to approach this.

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 🙂

Yeah, it might be worth adding some notes to say which parts will later be replaced when doing the initial PR.

@dario-piotrowicz
Copy link
Member

@james-elicx we've discussed this on the team and we'd be grateful if you would go ahead with the refactoring 🙂 😃
(with three PRs as we discussed if possible 🙂)

@james-elicx
Copy link
Contributor Author

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)

/index route

Problem

The build output config does not rewrite requests from / to /index, meaning that index pages will not be hit unless we intervene to ensure it does. The only instance where the build output config rewrites index pages is when there is an rsc header present, at which point it rewrites / to /index.rsc.

Solution

When extracting a list of routes from the file system, make a note of the parent to the index.func directory. e.g. /, or /base-path. We should add an additional record to the map of path names -> entrypoints that overrides any existing entry for / or /base-path, etc., and instead maps it to the entry point for /index.func instead.

Example
File structure with no base path could be /index.func, /about.func. We takes /index.func and identify it's parent to be /. In the map, we add / -> /index.func/index.js, as well as /index -> /index.func/index.js. Essentially, we add the index entrypoint a second time to be hit for / as well as /index, so that it is able to match requests to / to the /index route correctly as well as matching /index to /index.

Initial solution I wrote that I think is probably too excessive and makes less sense than the solution above.

When extracting a list of routes from the file system, make a note of the parent to the index.func directory. e.g. /, or /base-path. During the routing system, we should then take these custom rewrites, and apply them at the end of the no handler phase and the start of the resource phase. This would then mean that if we are making a request to /, we rewrite that request to target the correct /index file so that it can match it correctly when it checks for the path name in the file system. If after the routing cycle completes and at some point it happened that a request was rewritten to /, it then matches again in the resource phase as that is run after all rewrites occur.

Example
Without a base path, the file structure could be /index.func, /about.func, etc. It would indicate that for /index.func, / is its parent. We would insert at the end of the no handler route a source route for / that would rewrite it to /index, so that we can match /index in the file system.

{
    "src": "^/$",
    "dest": "/index",
    "continue": true
}

The file structure with a base path could be /base/index.func, /base/about.func, /base.func etc. We would take /base/index.func, make note that the parent directory is /base, and insert into the parsed build output config a custom source route at the end of the no handler phase. This rewrites the request to instead of targeting a nodejs /base.func, it targets /base/index.func, and we can ignore the /base.func entirely during build time.

{
    "src": "^/base$",
    "dest": "/base/index",
    "continue": true
}

Middleware

Problem

The build output can, in some instances, create two middleware files. This is primarily the case with a base path. It creates a normal middleware function inside the base path, which has the .vc-config.json that points to a compiled index.js, and creates an additional middleware function outside of the base path with the .vc-config.json pointing to an uncompiled middleware.js.

Solution

Discard any functions which target middleware.js as the entry point.

The second uncompiled middleware should theoretically never be called as it appears to be created either outside of the location that middleware needs to run, or it is created as a second file next to the compiled middleware. In the latter case, it does not cause a problem as it is not the targetted entry point. In the former case, it causes a problem as we are not able to build this into our worker file, so the only solution is to discard this middleware function file.

This should, in theory, not cause any problems as the middleware we would be discarding/ignoring is created outside of the base path directory, for example, and all requests should be going to the base path, not outside it.

Base path

Problem

When using a base path, the Vercel build output builds a second function for the base path itself, i.e. /base-path.func. This function is a nodejs function though, and one that seemingly would never get called, other than to requests to /base-path, which should instead actually be hitting /base-path/index. In the case of when you are using middleware and an x-nextjs-data header is present, it attempts to rewrite /base-path/index to /base-path, then to a static file that doesn't exist, and then back to /base-path/index. So, this entry would not be hit, unless directly requesting /base-path, despite not being the correct handler for /base-path.

The other problem with using a base path is the middleware problem outlined above.

Build output screenshots

image
When middleware is present:
image
image
image

Solution

The /index route solution would fix this problem.

Internationalization (i18n)

Problem

Using i18n in next.config.js creates a nodejs function for each locale in the build output. The build output config then rewrites all requests to /$nextLocale/....

Build output screenshots

image
image

Solution

Option 1 (strongly preferred):
Don't support the next.config.js option for i18n as Next.js are trying to move away from supporting this option. Note that the app directory wants developers to do their own internationalization implementation instead.

Option 2 (not preferred):
Strip the /$nextLocale part from the destination rewrites and instead add the locale as a search parameter and send the user to the normal route with the locale as a search parameter. This would be incredibly hacky to solely support people building with legacy Next.js versions.

@dario-piotrowicz
Copy link
Member

@james-elicx thanks a lot for the ultra detailed explanation 🙂

These are my personal thoughts:

/index route

Your solution makes sense to me 🙂👍

your initial solution

Your initial solution does indeed seem a bit overcomplicated and it may make things more complicated also from an understanding/debugging point of view (like you might forget that you're adding those ad-hoc/custom rewrites and wonder where they come from etc...) (probably not likely but still, I'm just kind of saying that if possible it might be quite valuable to distinguish between Next.js/Vercel logic and our own)

One extra thing I would like to ask you is regarding the basePath, instead of identify it from the files structure, could we just read it from the project's next.config.js file instead? (if that were possible it does seem more robust and simple than relying on the files structure)

(I haven't tried but I guess it should be possible... well, anyways you can have an abstraction util like getBasePath and implement it in your preferred way and that should be quite easy to change later anyways)

Middleware

Again this seems totally sensible to me 👍

Base path

Thanks for the info, I didn't know about any such node function 😕

Seems all good since the /index solution is going to fix this anyways 🙂

If we want to make sure that the node function never gets hit anyways I think we could also do:

  • get the current basePath (via files structure or next.config.js as mentioned above)
  • check if there is a basePath.func directory, if there is one check its .vc-config, if its runtime is not edge then delete the directory

I think it could be nice to clean the function up and make sure we don't then create some incorrect entries/etc...
But it might not be needed at all, let me know what you think

Internationalization (i18n)

I think I am pretty fine with solution 1, solution 2 as you mentioned is pretty hacky and would also add maintenance costs.

I would definitely go with solution 1 for now, that also is not set in stone, I think we can always change our mind and go with solution 2 later if that seems needed right?

What I think we should then make sure works well is the next-intl which we can suggest instead of the built-in i18n solution right?
(ps, next-intl may currently not work: #28)
(of course this is out of scope for this issue)

Some Extra thoughts

The ad-hoc fixes should be well documented

One thing I would like to add is that for stuff all the above behaviors where we have to deal with things in an ad-hoc kind of way I think it would be extremely valuable to have detailed comments about the situation (otherwise it will get very ambiguous why some stuff is there and what it does).

Those comments should also point to the vercel/next source code to show where they are doing what they are (so that we can keep track of it and potentially if they change behaviors we can adapt to that). But if pinpointing the source code is problematic or too much time consuming I think this shouldn't block your PR and you could add TODO comments there saying that further investigation/documentation on that is needed.

Testing

You did mention that you're going to work on testing as well, I do think that that would be quite valuable so try to do so 🙂, but just so that you are aware, we're working on implementing some e2e tests soon, so keep that in mind and maybe make a note or something of things that could be tested with those as well (or instead of unit/integration tests). For example the / routing would be trivial with an e2e, although it should be easy enough with a unit test as well, but you get what I mean

@james-elicx
Copy link
Contributor Author

james-elicx commented Apr 4, 2023

One extra thing I would like to ask you is regarding the basePath, instead of identify it from the files structure, could we just read it from the project's next.config.js file instead? (if that were possible it does seem more robust and simple than relying on the files structure)

(I haven't tried but I guess it should be possible... well, anyways you can have an abstraction util like getBasePath and implement it in your preferred way and that should be quite easy to change later anyways)

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.

If we want to make sure that the node function never gets hit anyways I think we could also do:

  • get the current basePath (via files structure or next.config.js as mentioned above)
  • check if there is a basePath.func directory, if there is one check its .vc-config, if its runtime is not edge then delete the directory

I think it could be nice to clean the function up and make sure we don't then create some incorrect entries/etc... But it might not be needed at all, let me know what you think

Yeah, so the idea was that when the /base entry is mapped to /base/index.func/index.js, it would completely overwrite any entry for /base, which means that this nodejs function entry would be discarded, so deleting the directory wouldn't really have been necessary, although that is still something that can be done.

I would definitely go with solution 1 for now, that also is not set in stone, I think we can always change our mind and go with solution 2 later if that seems needed right?

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).

What I think we should then make sure works well is the next-intl which we can suggest instead of the built-in i18n solution right? (ps, next-intl may currently not work: #28) (of course this is out of scope for this issue)

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).

The ad-hoc fixes should be well documented

One thing I would like to add is that for stuff all the above behaviors where we have to deal with things in an ad-hoc kind of way I think it would be extremely valuable to have detailed comments about the situation (otherwise it will get very ambiguous why some stuff is there and what it does).

Those comments should also point to the vercel/next source code to show where they are doing what they are (so that we can keep track of it and potentially if they change behaviors we can adapt to that). But if pinpointing the source code is problematic or too much time consuming I think this shouldn't block your PR and you could add TODO comments there saying that further investigation/documentation on that is needed.

Very strongly agree with well documenting it. As to the source code, I'll take a look, but no promises there.

Testing

You did mention that you're going to work on testing as well, I do think that that would be quite valuable so try to do so 🙂, but just so that you are aware, we're working on implementing some e2e tests soon, so keep that in mind and maybe make a note or something of things that could be tested with those as well (or instead of unit/integration tests). For example the / routing would be trivial with an e2e, although it should be easy enough with a unit test as well, but you get what I mean

Understood. I was planning on just writing some unit tests for the parts I add.

@dario-piotrowicz
Copy link
Member

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.

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.
But yeah let's just use the build output and not rely on Next.js specific details at all 👍 (we can always change that later if needed)

(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).

ah yeah I kind of thought that might be the case (the issue is that the _redirects file won't be in the .vercel/output/static dir does won't get saved and picked up at runtime right?), but if we really wanted we could make it work right? (something akin to buildMetadataFiles)

Very strongly agree with well documenting it. As to the source code, I'll take a look, but no promises there.

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 🙂

Understood. I was planning on just writing some unit tests for the parts I add.

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 🙂👍

@james-elicx
Copy link
Contributor Author

james-elicx commented Apr 4, 2023

(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).

ah yeah I kind of thought that might be the case (the issue is that the _redirects file won't be in the .vercel/output/static dir does won't get saved and picked up at runtime right?), but if we really wanted we could make it work right? (something akin to buildMetadataFiles)

Well, what he did appears to just manually be adding a redirect from / to /vi. This is something that would automatically be done with the build output config, but, then we would be trying to resolve a route at /vi which, in my above example, the locales ended up being built to a nodejs function by Next.js/Vercel. And the build output config rewrites requests to /$nextLocale/....

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 / page to /vi (which I don't believe actually has a matcher in the middleware manifest, so not sure how it's matching unless he made an actual page for it), but does not account for any other pages and therefore the other pages should just be using / instead, meaning they resolve their entry points to the regular entry points and no i18n support is given.

@james-elicx
Copy link
Contributor Author

james-elicx commented Apr 4, 2023

Revised middleware solution

I have identified a third case for built middleware 🙄

  • Builds a function targeting a compiled index.js file.
  • Builds a function targeting an uncompiled middleware.js file.
  • Builds a function targeting an uncompiled middleware.js file when there is a compiled index.js file in the directory.

If a middleware has a .vc-config.json that targets an uncompiled middleware.js entry point, we should first check and see if there is a compiled index.js in the directory that we can target as the entry point. If there is, use that compiled file as the entry point, but if there isn't, then this middleware should be safe to discard as it cannot be used in its uncompiled form.

This should fix the issue from #130 where it appears to sometimes target the uncompiled version instead of the compiled version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants