fix: normalize the rawPath without locale before attempting to asserting the 404 page #448
Conversation
🦋 Changeset detectedLatest commit: 8bb1728 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
I've updated the examples/pages-routes project to include some i18n config. I looked at the current e2e and they would already cover this bug since they assert the data json calls are resolving. Would that be sufficient enough? |
|
Thanks for adding the configs. I think it'll be useful to test the actual internationalization of the e2e apps to, that is, to add |
Hmm, looking at the example I'm not sure it would work seeing that the middleware takes care of all the i18n stuff with the app router. This means that I wouldn't know the i18n configuration to start with so my new "code" would never be executed 🤔 TBH not using the app router at all atm, we stick with good old Pages 🤷 |
|
It shouldn't be necessary to change anything for app dir, the middleware rewrites should already be applied at this point |
| // Add the locale prefix to the regex so we correctly match the rawPath | ||
| const optionalLocalePrefixRegex = !!RoutesManifest.locales.length | ||
| ? `^/(?:${RoutesManifest.locales.map((locale) => `${locale}/?`).join("|")})?` | ||
| : null; |
There was a problem hiding this comment.
This make pnpm run build fail for me, in case people don't have i18n it should be the same so ^/
| @@ -22,12 +22,17 @@ export interface MiddlewareOutputEvent { | |||
| origin: Origin | false; | |||
| } | |||
|
|
|||
| // Add the locale prefix to the regex so we correctly match the rawPath | |||
| const optionalLocalePrefixRegex = !!RoutesManifest.locales.length | |||
| ? `^/(?:${RoutesManifest.locales.map((locale) => `${locale}/?`).join("|")})?` | |||
There was a problem hiding this comment.
This cause an issue with 3 e2e test because of this, it should also include the default case (i.e. with no locales prefix)
For the next data test it is because of this https://github.com/sst/open-next/blob/9d632d1bd5c3a8d7e7bbd13ae9daaca68da9284e/packages/tests-e2e/tests/pagesRouter/data.test.ts#L13 It ends now with en.json
There was a problem hiding this comment.
What would be the best approach here? Create a new pages app with i18n + additional e2e tests? 🤔 . It's expected that in this case it's en.json instead of index.json (but only if you have i18n settings).
It's a bit of a mess (on Next.js side)
There was a problem hiding this comment.
In a perfect world, yeah we should do something like that, but honestly just testing with en.json is probably enough
No description provided.