Fix: react resolution for interception, parallel and group routes #155

conico974 · 2023-07-06T15:13:35Z

Right now with app dir, interception, parallel and group routes doesn't work correctly.
This PR should solve this

changeset-bot · 2023-07-06T15:13:38Z

⚠️ No Changeset found

Latest commit: 1fc4ec5

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

vercel · 2023-07-06T15:13:39Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
open-next	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 29, 2023 10:20am

khuezy · 2023-07-17T22:35:38Z

@conico974 This appears to be broken in latest 13.4.10 😢
vercel/next.js#50243 (see latest posts)

khuezy · 2023-07-26T22:29:27Z

packages/open-next/src/adapters/server-adapter.ts

+  // Get route pattern
+  if (rewrite) {
+    route = {
+      page: rewrite.destination,


If you have dynamic routes, the destination is /:dynamic, while the routes manifest is /[dynamic], and thus isApp will fail and cause 404 errors.

rewrite.destination.replace(/:([^\/]+)/g, '[$1]')

khuezy

Great work! Fixes a few issues others have reported w/ the query stuff too 👍
I'll test this in a couple hours.

packages/open-next/src/adapters/routing.ts

khuezy · 2023-07-30T14:25:36Z

packages/open-next/src/adapters/routing.ts

+  }
+  // Do we need to replace anything else here?
+  const rewrittenUrl =
+    rewrite?.destination.replace(/:[^\/]+/g, '[$1]') ?? rawPath;


I think that should be good, I've checked the Vercel deployed app and they contain the (.) in the route (so we don't need to replace the groups; they may change that in the future for all we know)

khuezy · 2023-07-30T14:31:42Z

packages/open-next/src/adapters/routing.ts

+  }
+  // Do we need to replace anything else here?
+  const rewrittenUrl =
+    rewrite?.destination.replace(/:[^\/]+/g, '[$1]') ?? rawPath;


Suggested change

rewrite?.destination.replace(/:[^\/]+/g, '[$1]') ?? rawPath;

rewrite?.destination.replace(/:([^\/]+)/g, '[$1]') ?? rawPath;

You forgot the grouping (), otherwise, it'll replace the word with a literal $1

Yeah i noticed that, testing this right now

This wasn't working finally, we need to replace the rawPath here, i used https://github.com/pillarjs/path-to-regexp. This is the lib they use internally inside next

Hmm I'm getting 502's on prefetch now
Invoke Error {"errorType":"TypeError","errorMessage":"Expected \"0\" to be a string","stack":["TypeError: Expected \"0\" to be a string"," at file:///var/task/packages/app/index.mjs:27:37895"

It's likely that it comes from here https://github.com/conico974/open-next/blob/b3060a37960e4a1305fe82d125d5003c7e0897be/packages/open-next/src/adapters/routing.ts#L74-L85. Could you try adding some log here to see what could be the cause

I updated my comment w/ additional info.

I should have read the docs a little more... https://github.com/pillarjs/path-to-regexp#unnamed-parameters. So in compile it treats (.) as an unnamed parameter, and so it expects it to be present when we use it in toDestination. see https://github.com/pillarjs/path-to-regexp/blob/1cbb9f3d9c3bff97298ec45b1bb2b0beb879babf/src/index.spec.ts#L2892
We actually need the interception path (i.e. (.)|(..)|(...)) to be returned from our function so that it return the correct route. I think we need to escape those interception path for when we use toDestination and then replace those escaped path with (.)|(..)|(...) so that we return the expected path

Hmm but isn't that (.) regex, and not directory paths?

I added a util function in utils.ts:

const replacePathRegex = /($?\w+$?\/)?$\.{2}$\/?/; export function resolvePaths(path: string) { const rootIndex = path.indexOf("(...)"); // If path contains (...), replace everything before it, including itself if (~rootIndex) { path = path.substring(rootIndex + 5); } // Remove (.), since that resolves to nothing path = path.replace(/$\.$\/?/g, ""); // Resolves each (..) path by consuming the path before it // eg: /page/(..)[dynamic] => /[dynamic] while (path.match(/$\.{2}$/)) { path = path.replace(replacePathRegex, ""); } return path; }

Then in routing.ts, I resolved the source & destination:

const toDestination = compile(resolvePaths(rewrite?.destination) ?? ""); const fromSource = match(resolvePaths(rewrite?.source) ?? "");

This works pretty well (for now, needs more testing)

Nvm, the modal's param.title is messed up for some reason, it has %3Atitle
The client side params is:

author: "%3Aauthor" title: "%3Atitle"

The object returned has url: url: '/page/(.):author/:title?_rsc=3sl5w',

khuezy · 2023-07-30T19:57:06Z

packages/open-next/src/adapters/routing.ts

+      if (isUsingParams) {
+        rewrittenUrl = toDestination(params);
+      }
+      rewrittenUrl = rewrite.destination;


This should be inside an else statement, otherwise the toDestination gets overridden. Hmmm, modal breaks again w/ the else statement.

Ho come on, how could i forget that..
Ok at least this way it makes more sense to me, i couldn't understand how your fix could resolve the issue, i'm pretty sure we need the (.) in the final url.
I've just tried something and it seems to work

I created these 2 functions

export function escapeRegex(str: string) { let path = str.replace(/$\.$/g, '%1'); path = path.replace(/$\.{2}$/g, '%2'); path = path.replace(/$\.{3}$/g, '%3'); return path; } export function unescapeRegex(str: string) { let path = str.replace(/%1/g, '(.)'); path = path.replace(/%2/g, '(..)'); path = path.replace(/%3/g, '(...)'); return path; }

And for the rewrite part

const toDestination = compile( escapeRegex(rewrite?.destination ?? '') ?? '' ); const fromSource = match(escapeRegex(rewrite?.source) ?? ''); const _match = fromSource(rawPath); if (_match) { const { params } = _match; const isUsingParams = Object.keys(params).length > 0; if (isUsingParams) { rewrittenUrl = unescapeRegex(toDestination(params)); console.log('rewrittenUrl1', rewrittenUrl); } else { rewrittenUrl = rewrite.destination; } console.log('rewrittenUrl2', rewrittenUrl);

Of course those escape character are pretty bad, but for testing it's ok, i need to figure out some escape that will not mess things up

I have an extremely ugly hack to get the correct params from the modal:

In routing.ts:

let params: { [key: string]: string } = {}; if (rewrite) { const fromSource = match(resolvePaths(rewrite?.source) ?? ""); const _match = fromSource(rawPath); if (_match) { params = _match.params as any; rewrittenUrl = rewrite.destination; } } return { rawPath: rewrittenUrl, url: `${rewrittenUrl}${urlQueryString ? `?${urlQueryString}` : ""}`, params, __rewrite: rewrite, };

In server-adapter:

const { rawPath, url, params } = handleRewrites( internalEvent, routesManifest.rewrites ); .... const body = ServerResponse.body(res) .toString(encoding) .replace(/%3A([^"]+)/g, (_, match: string) => { return params[match] || `%3A${match}`; });

But there's gotta be a correct way to do this...

Cool, let me give that a go.

Nice! Modal interception + client parameters works now :)

Would fromSource response ever need to be unescapeRegex'd?

Yes otherwise next-server will not find the correct interception route path.
I'll change the escape character to something like_µ1_, it should be safe enough so that user will not have these in the url

khuezy · 2023-07-30T21:11:48Z

I tried rewrite and redirect. Redirect works but rewrite returns 404... I'll debug some more. My rewrite config is:

async rewrites() {
  return [{source: '/about', destination: '/'}]
}

This works locally but 404s on deployed app.

conico974 · 2023-07-30T21:20:55Z

I tried rewrite and redirect. Redirect works but rewrite returns 404... I'll debug some more. My rewrite config is:
async rewrites() {
  return [{source: '/about', destination: '/'}]
}
This works locally but 404s on deployed app.

It might be that it's generated as afterFiles or as fallback (see https://github.com/conico974/open-next/blob/aec5166911b7887735d8546dbc108bd86bdf7a6f/packages/open-next/src/adapters/util.ts#L48). I'm not sure how we should handle that, especially when.

I tested mine this way and it works for me, even with dynamic path

rewrites: () => ({
    beforeFiles: [
      {
        source: '/shouldBeRewritten',
        destination: '/',
      },
      {
        source: '/shouldBeRewritten/:path',
        destination: '/:path',
      },
    ],
  }),

khuezy · 2023-07-30T21:25:17Z

Ah yes, it's generated as afterFiles. I did this and it worked so far:

  const { beforeFiles, afterFiles, fallback } = routesManifest.rewrites;
  const rewrites = [...beforeFiles, ...afterFiles, ...fallback];

khuezy · 2023-08-06T18:11:59Z

packages/open-next/src/adapters/util.ts

+    cleanedKey = cleanedKey.replace(/\/\((?!\.)[^\)]*\)/g, '');
+
+    // Remove /page suffix
+    cleanedKey = cleanedKey.replace(/\/page$/g, '');


@conico974 I'm using app/api/something/route.tsx, but the key still contains app/api/something/route, should we also remove the route? The nextjs codebase deletes the last /<thing> from the key.

Technically we should, but I don't think it matters that much. This is used only to modify react resolution so that your page use the same version of react as next. I don't think they do anything related to react on route

Some people are having rewrite issues in next config. I think we should concat before/after files and fallback unless there's a gotcha w/ it. See: https://discord.com/channels/983865673656705025/1138321896816001086

There is actually a gotcha, https://nextjs.org/docs/app/building-your-application/routing/middleware#matching-paths. We should follow this execution order.
I think in order to do that we'll have to merge this with your middleware fix, i'll try to come up with something later today

khuezy · 2023-09-01T21:54:36Z

#208

seed-deploy bot had a problem deploying to pr155 July 6, 2023 15:13 Failure

vercel bot deployed to Preview July 6, 2023 15:14 View deployment

seed-deploy bot had a problem deploying to pr155 July 11, 2023 09:18 Failure

vercel bot deployed to Preview July 11, 2023 09:19 View deployment

conico974 marked this pull request as ready for review July 11, 2023 09:23

khuezy reviewed Jul 26, 2023

View reviewed changes

conico974 force-pushed the fix/parallel-routes branch from f866acf to 5abab09 Compare July 30, 2023 13:56

seed-deploy bot had a problem deploying to pr155 July 30, 2023 13:56 Failure

vercel bot deployed to Preview July 30, 2023 13:57 View deployment

khuezy reviewed Jul 30, 2023

View reviewed changes

seed-deploy bot had a problem deploying to pr155 July 30, 2023 15:27 Failure

vercel bot deployed to Preview July 30, 2023 15:28 View deployment

khuezy reviewed Jul 30, 2023

View reviewed changes

seed-deploy bot had a problem deploying to pr155 July 30, 2023 21:07 Failure

vercel bot deployed to Preview July 30, 2023 21:08 View deployment

khuezy reviewed Aug 6, 2023

View reviewed changes

seed-deploy bot temporarily deployed to pr155 August 23, 2023 14:41 Inactive

vercel bot deployed to Preview August 23, 2023 14:41 View deployment

conico974 added 5 commits August 29, 2023 10:21

fix: fix react resolution for interception, parallel and group routes

b4631be

fix: group & parallel routes should be removed first

2a4c32f

fix: interception use rewrites

2b6908f

fix: rewrites could be empty

bfb42bc

fix: handle rewrites internally

dd5d60f

conico974 added 5 commits August 29, 2023 10:34

fix: handle redirects

aa44214

fix: rewrite route for dynamic params

cf74c1f

fix: escape interception regex

57284a4

feat: add external url redirect & rewrite

023f707

fix: add afterFiles and fallback

1fc4ec5

conico974 force-pushed the fix/parallel-routes branch from 12512bf to 1fc4ec5 Compare August 29, 2023 10:19

vercel bot deployed to Preview August 29, 2023 10:20 View deployment

khuezy closed this Sep 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: react resolution for interception, parallel and group routes #155

Fix: react resolution for interception, parallel and group routes #155

conico974 commented Jul 6, 2023

changeset-bot bot commented Jul 6, 2023 •

edited

Loading

vercel bot commented Jul 6, 2023 •

edited

Loading

khuezy commented Jul 17, 2023

khuezy Jul 26, 2023

khuezy left a comment

khuezy Jul 30, 2023

khuezy Jul 30, 2023

conico974 Jul 30, 2023

conico974 Jul 30, 2023

khuezy Jul 30, 2023 •

edited

Loading

conico974 Jul 30, 2023

khuezy Jul 30, 2023

conico974 Jul 30, 2023

khuezy Jul 30, 2023

khuezy Jul 30, 2023 •

edited

Loading

khuezy Jul 30, 2023 •

edited

Loading

khuezy Jul 30, 2023 •

edited

Loading

conico974 Jul 30, 2023

khuezy Jul 30, 2023

khuezy Jul 30, 2023

khuezy Jul 30, 2023

khuezy Jul 30, 2023

conico974 Jul 30, 2023

khuezy commented Jul 30, 2023 •

edited

Loading

conico974 commented Jul 30, 2023

khuezy commented Jul 30, 2023 •

edited

Loading

khuezy Aug 6, 2023

conico974 Aug 7, 2023

khuezy Aug 8, 2023

conico974 Aug 9, 2023

khuezy commented Sep 1, 2023

	rewrite?.destination.replace(/:[^\/]+/g, '[$1]') ?? rawPath;
	rewrite?.destination.replace(/:([^\/]+)/g, '[$1]') ?? rawPath;

Fix: react resolution for interception, parallel and group routes #155

Fix: react resolution for interception, parallel and group routes #155

Conversation

conico974 commented Jul 6, 2023

changeset-bot bot commented Jul 6, 2023 • edited Loading

⚠️ No Changeset found

vercel bot commented Jul 6, 2023 • edited Loading

khuezy commented Jul 17, 2023

Choose a reason for hiding this comment

khuezy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khuezy Jul 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khuezy Jul 30, 2023 • edited Loading

Choose a reason for hiding this comment

khuezy Jul 30, 2023 • edited Loading

Choose a reason for hiding this comment

khuezy Jul 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khuezy commented Jul 30, 2023 • edited Loading

conico974 commented Jul 30, 2023

khuezy commented Jul 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khuezy commented Sep 1, 2023

changeset-bot bot commented Jul 6, 2023 •

edited

Loading

vercel bot commented Jul 6, 2023 •

edited

Loading

khuezy Jul 30, 2023 •

edited

Loading

khuezy Jul 30, 2023 •

edited

Loading

khuezy Jul 30, 2023 •

edited

Loading

khuezy Jul 30, 2023 •

edited

Loading

khuezy commented Jul 30, 2023 •

edited

Loading

khuezy commented Jul 30, 2023 •

edited

Loading