[next] Fix nextjs routes in `vercel dev` #4510

jeantil · 2020-05-27T21:12:23Z

This change fixes #4239 where using now dev to work on monorepos having
a next.js app which is not in the topmost directory fails to correctly route
to dynamic pages.

jeantil · 2020-05-27T21:20:35Z

I have tried this change locally and deployed on vercel using a staging deployment of our production application. I was unable to detect any adverse effect as both worked fine.

The unit tests ran fine
The integration tests failed with a seemingly unrelated error:

Error! No example found for angular, run `vercel init` to see the list of available examples.
1
  ✖ initialize example "angular" Received:
  "Vercel CLI 19.0.2-canary.7 — https://vercel.com/feedback
  - Fetching examples
"
  "Error! No example found for angular, run `vercel init` to see the list of available examples."
$ vercel logout -Q /tmp/tmp-96122GO0HfKwxo5Zn/com.vercel.tests

  1 test failed

  initialize example "angular"

  /home/jean/dev/startups/yupwego/src/now/packages/now-cli/test/integration.js:1812

   1811:                                                                   
   1812:   t.is(exitCode, 0, formatOutput({ stdout, stderr }));            
   1813:   t.true(stdout.includes(goal), formatOutput({ stdout, stderr }));

  Received:
  "Vercel CLI 19.0.2-canary.7 — https://vercel.com/feedback
  - Fetching examples
"
  "Error! No example found for angular, run `vercel init` to see the list of available examples."

  Difference:

  - 1
  + 0

As far as I could tell, before my change, isDev could only be false or undefined in getDynamicRoutes, this means that all the !isDev checks would return true even if the app was running within now dev in particular:

https://github.com/vercel/vercel/blob/master/packages/now-next/src/utils.ts#L381
https://github.com/vercel/vercel/blob/master/packages/now-next/src/utils.ts#L445
the second is explicitely preceded by a comment that states:

// in now dev we don't need to prefix the destination

Since my change isDev is propagated from the build signature and will be true if running the builder from now dev. As I said, I was unable to identify adverse effects with this change.

hopefully I missed something

ijjk · 2020-05-29T16:17:18Z

Hi, can you provide a link to an example project this is trying to fix? This legacy routes generation should no longer be relied on since it conflicts with the new rewrites/redirects support in Next.js.

I think the correct way to handle this would be to make sure we proxy all request to next dev in the same way we do for zero config projects e.g. without builds defined or use next dev directly without vc dev if possible

jeantil · 2020-05-30T10:04:01Z

Hi @ijjk

I have quickly configured an example in https://github.com/jeantil/next-9-ts-aliases-workspaces/tree/vercel/dynamic-paths-failure which demonstrates the issue:
deploy to vercel,
Try to access

/
/another?word=foo
/users/1

run vercel dev
Try to access

/
/another?word=foo
/users/1

I get a 404 on the last one in dev and with @styfle's guidance, I tracked it down to this modification. Using a builder with this modification in both deployed and dev work fine.
Note that the example is contrived, since another-app is a trivial bit of js, I am aware it could be moved to web-app\pages\api. However in production apps, another-app could also be written in go, ruby or python which cannot be integrated as is in web-app ...

This legacy routes generation should no longer be relied on since it conflicts with the new rewrites/redirects support in Next.js.
I think the correct way to handle this would be to make sure we proxy all request to next dev in the same way we do for zero config projects e.g. without builds defined or use next dev directly without vc dev if possible

I try to follow the changes in both vercel and next but I was unaware of this feature.
I looked it up and as far as I know custom routes in next.js are not available yet. The RFC is still open, there has been no mention of it in the announcements either on vercel or next.js. It may be the correct way at some point in the future, but it will likely require upgrading nextjs which usually means a bunch of changes which is not trivial in an actively developped production app and in the meantime dynamic routes are broken on vercel dev ...

jeantil · 2020-06-08T09:41:58Z

Hello @styfle @ijjk , have you had time to review this issue further ? we are not feeling very safe diverging from vercel published builders for too long, maintaining our own fork is quite time consuming but dynamic next routes are broken in our project (and probably a few others) without this fix.
The test failures in the CI all mention that the failure is expected for 3rd party contributions. I tried running the cli integration tests locally with a VERCEL_TOKEN and I got seemingly unrelated failures

 ✖ output the version 
$ vercel logout -Q /tmp/tmp-15516GU9bdMhrmAQ1/com.vercel.tests

  1 test failed

  output the version

  /home/jean/dev/startups/yupwego/src/vercel/vercel/packages/now-cli/test/integration.js:678

   677:   t.truthy(semVer.valid(version));
   678:   t.is(version, pkg.version);     
   679: });                               

  Difference:

  - '19.0.2-canary.11'
  + '19.0.2-canary.13'

and now repeating the same command

yarn test-integration-cli --clean false

fails right at the start, I'm not sure what's wrong and I'm quite lost in the repo setup, some guidance would be most welcome.

This PR fixes a longstanding issue introduced in #3673 that prevents routing properties from applying to the framework's upstream dev server. This mimic's the older proxy logic used in build matches here: https://github.com/vercel/vercel/blob/5035fa537f82181649d42778c60145d8890f3291/packages/now-cli/src/util/dev/server.ts#L1535-L1539 - Related to #3777 - Related to #4510

jeantil · 2020-06-17T11:57:13Z

@styfle I have looked at #4644 and installed 19.1.1 locally to see if it made this PR obsolete.

Unfortunately #4644 does not fix the nextjs dynamic routing in vc dev. we still get a 404 from vercel (not the one from next.js) for any route which has a dynamic component.

Is there any concern regarding this PR that I need to address to help it move forward, it makes our DX really bad. We use a custom build of @vercel/next in dev to get the fix from this PR, but it's quite a costly process so getting this in would free up a lot of effort for us

styfle · 2020-06-17T14:58:47Z

Hi @jeantil

Thanks for the PR!

The only thing missing is a test fixture. You can add one to ./packages/now-cli/test/dev/fixtures, perhaps just your example repo you linked to earlier.

You can run locally by setting your VERCEL_TOKEN env var and running yarn build and then yarn test-integration-dev. Note that this will create test deployments on your account so you can change the integration.js file to use test.only instead of test so it only runs a single test deployment.

styfle · 2020-06-17T15:34:26Z

Actually, let's put the new test in ./packages/now-cli/test/dev/fixtures since this is a vercel dev test.

I updated my comment above.

ijjk · 2020-06-17T15:43:09Z

packages/now-next/src/index.ts

+        urls[entrypoint],
+        meta.isDev


Suggested change

urls[entrypoint],

meta.isDev

urls[entrypoint]

This method is only used in dev mode so we don't need to pass whether we're in dev mode or not

@ijjk I think its better to be explicit, someone might move this getRoutes() to a different place where it might get used in production.

hmm i reverted this to resolve this comment but I can re-revert :)

packages/now-next/src/utils.ts

packages/now-next/src/index.ts

ijjk · 2020-06-17T15:44:59Z

packages/now-next/src/utils.ts

@@ -490,7 +495,7 @@ export async function getDynamicRoutes(
      routes.push({
        src: pageMatcher.matcher.source,
        dest,
-        check: true,
+        check: !isDev,


Is this change needed? Seems like it shouldn't affect the routes in dev mode 🤔

Actually that change is the fix, all the rest was just propagating the value of isDev though the function calls because I failed to notice that getRoutes was always only called in dev mode.

I initially added tons of traces in the route matching code and ended up with traces like

dev router trying to match undefined /blog/top-10-des-incontournables-destination-du-perou dev router trying to match GET /www/blog/top-10-des-incontournables-destination-du-perou to {"src":"^/www/(blog/([^/]+?)(?:/)?)$","dest":"http://localhost:44659/$1","check":true} =============== hasDestFile false pathname /blog/top-10-des-incontournables-destination-du-perou destPath http://localhost:44659/blog/top-10-des-incontournables-destination-du-perou phase null routeConfig {"src":"^/www/(blog/([^/]+?)(?:/)?)$","dest":"http://localhost:44659/$1","check":true} ===============

After a bit of back and forth testing a few vercel.json configurations @styfle wrote:

Oh so the route ` {"src":"^/www/(blog/([^/]+?)(?:/)?)$","dest":"http://localhost:43097/$1","check":true} is from @vercel/next which seems wrong. I dont think it should have check:true

I traced it back to this line and indeed changing it to false resolved the issue. I must confess that I have absolutely no idea what check: true orcheck: false does. since I needed is to be false in dev to get the behaviour I wanted, I made the safest possible change I could by making sure this only affected dev mode.

on a side note, since the value of the isDev parameter was not provided to getDynamicRoutes before this change isDev was always undefined, thus !isDev was always true, thus the code at https://github.com/vercel/vercel/pull/4510/files#diff-f5c41b908e3f767349246f7dfb3714e1L484 was always going through path.join('/', entryDirectory, pageMatcher.pageName) but I was unable to detect an impact after changing it. It is worth noting that just fixing the isDev value without setting check to false was not enough to fix the issue.

@ijjk The problem is that check: true is similar to continue: true when the resource is not found. See the routing logic here:

vercel/packages/now-cli/src/util/dev/router.ts

Line 120 in d7731d1

if (routeConfig.check && devServer && phase !== 'hit') {

In particular, this is where it continues:

vercel/packages/now-cli/src/util/dev/router.ts

Line 149 in d7731d1

continue;

packages/now-cli/test/dev/fixtures/monorepo-dynamic-paths/blog/next.config.js

jeantil · 2020-06-17T22:09:46Z

I have added the integration test in packages/now-cli/test/dev/fixtures/monorepo-dynamic-paths/ unfortunately the test cannot pass as it must use builds which means it uses the packaged @vercel/next builder which doesn't contain the fix from this branch.

@styfle suggested I use @vercel/next@canary which will fix the test automatically once the PR is merged and released as a canary.

packages/now-cli/test/dev/fixtures/monorepo-dynamic-paths/blog/package.json

@styfle

This change fixes vercel#4239 where using `now dev` to work on monorepos where the next.js app is not in the topmost directory fails to correctly route to dynamic pages. After investigating the devServer router, @styfle prompted me to investigate the @vercel/next builder. He also suggested restricting `check` to be false only when running in `now dev`. This led me to realize that isDev was either false or undefined in getDynamicRoutes. Fixing that didn't lead to any observable adverse effect on my repository either running `yarn test-unit`, during `vercel dev` or while deploying on vercel...

jeantil · 2020-06-18T12:12:45Z

I managed to make the integration test work by deploying a custom build of now-next with the patch (next-2.6.8-canary.0.tgz) and forcing the integration test to use that. I then reverted vercel.json to use @vercel/next@canary and the test failed again.
I also verified manually with vercel dev that all was fine with the fix.

In the process of making the integration test work I discovered further breakage in the routing in prod mode:

packages/now-cli/test/dev/fixtures/monorepo-dynamic-paths/home/pages/[id].js which resolves to /[id] is a dynamic route which doesn't use getStaticProps but relies on useRouter as documented on the next.js website
packages/now-cli/test/dev/fixtures/monorepo-dynamic-paths/home/pages/1/[id].js which resolves to /1/[id] is a dynamic route which uses getStaticProps where fallback mode fails. I actually encountered this in production where it is mitigated by a custom nextjs based 404. somehow it shortly displays the 404 but it then loads the actual page (my unverified hypothesis is that it's linked to temporarily displaying a SSG 404 but then rehydrating the dynamic page and its fallback props)

Here is a table of observed results for the above routes, for vercel prod you can check this deployment

route	path	next dev	next start	vercel dev	vercel dev with pr	vercel prod	comment
/[id]	/foo	✔	✔	❌	✔	❌
/1/[id]	/1/dynamic	✔	✔	❌	✔	✔	exported by getStaticPaths
/1/[id]	/1/foo	✔	✔	❌	✔	❌	not exported by getStaticPaths

Interestingly next dev and vercel dev both display the correct page for the last case but both emit the following warning because of a next.js bug:

Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client
    at ServerResponse.setHeader (_http_outgoing.js:526:11)
    at DevServer.sendHTML (/home/jean/dev/startups/yupwego/src/vercel/vercel/packages/now-cli/test/dev/fixtures/monorepo-dynamic-paths/node_modules/next/dist/server/next-dev-server.js:23:5)
    at DevServer.render (/home/jean/dev/startups/yupwego/src/vercel/vercel/packages/now-cli/test/dev/fixtures/monorepo-dynamic-paths/node_modules/next/dist/next-server/server/next-server.js:49:37)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
    at async Object.fn (/home/jean/dev/startups/yupwego/src/vercel/vercel/packages/now-cli/test/dev/fixtures/monorepo-dynamic-paths/node_modules/next/dist/next-server/server/next-server.js:35:852)
    at async Router.execute (/home/jean/dev/startups/yupwego/src/vercel/vercel/packages/now-cli/test/dev/fixtures/monorepo-dynamic-paths/node_modules/next/dist/next-server/server/router.js:28:28)
    at async DevServer.run (/home/jean/dev/startups/yupwego/src/vercel/vercel/packages/now-cli/test/dev/fixtures/monorepo-dynamic-paths/node_modules/next/dist/next-server/server/next-server.js:44:494)
    at async DevServer.handleRequest (/home/jean/dev/startups/yupwego/src/vercel/vercel/packages/now-cli/test/dev/fixtures/monorepo-dynamic-paths/node_modules/next/dist/next-server/server/next-server.js:13:133) {
  code: 'ERR_HTTP_HEADERS_SENT'

next start doesnt have any error and doesn't emit any warning ...

--edit--
the warning disappears when using next@canary but the errors in prod are still there.

styfle

Thanks for all the work you put into this, great job! 🎉

I ran all the tests locally and found one that needed to be updated so I pushed a change.

I think we can merge this and address the other bugs in another PR 👍

jeantil · 2020-06-19T05:27:39Z

thank you !

morgs32 · 2020-07-11T12:41:07Z

Any issue/PR I can watch to know when these get resoled?

If not - a post here would do it!

jeantil · 2020-07-13T08:55:21Z

@morgs32 it would be worth investigating this table further since I wasn't aware of fallback pages at the time and it may explain some of the issue but I don't have time to do this for the moment

morgs32 · 2020-07-13T13:15:57Z

I understand @jeantil. How did you go about testing now-next? Did you publish your own version as you were debugging and adding logs? Is there a better way?

jeantil · 2020-07-13T15:40:47Z

I suggest building the full checkout once, then
cd ./packages/now-next && yarn build && npm pack
which will create a builder package you can link to.

in vercel.json you can then use a file://... reference which uses an absolute path to your newly built builder package, mine was "use": "file:/home/jean/dev/vercel/packages/now-next/vercel-next-2.6.3-canary.3.tgz",

if you suspect your modifications are not picked up, you can rm -r ~/.cache/com.vercel.cli/dev/builders/node_modules to delete the installed builders and force the cli to reinstall them.

I built the above table by experimenting with the testcase I added for this PR in packages/now-cli/test/dev/fixtures/monorepo-dynamic-paths/

good hunting !

morgs32 · 2020-07-16T14:16:24Z

Well I created an issue with vercel's website since I can't do it through GitHub. Here's a PR that got dynamic routes to work in my own deployment. Not sure it's a safe fix - but thought I'd share!
#4865

jeantil requested review from ijjk and Timer as code owners May 27, 2020 21:12

styfle added the @vercel/next label May 27, 2020

styfle changed the title ~~fixes dynamic nextjs page routing in now dev~~ [next] Fix dynamic nextjs page routing in vercel dev May 27, 2020

styfle changed the title ~~[next] Fix dynamic nextjs page routing in vercel dev~~ [next] Fix nextjs routes in vercel dev May 27, 2020

jeantil force-pushed the master branch from 7d181bd to 9ee5dd0 Compare May 29, 2020 10:07

jeantil force-pushed the master branch 2 times, most recently from f124515 to 2a730bc Compare June 8, 2020 09:32

TooTallNate requested a review from styfle June 8, 2020 17:33

styfle mentioned this pull request Jun 12, 2020

[cli] Fix vc dev proxy to upstream frontend dev server #4644

Merged

jeantil force-pushed the master branch from 2a730bc to a933b83 Compare June 17, 2020 12:04

ijjk reviewed Jun 17, 2020

View reviewed changes

packages/now-next/src/utils.ts Outdated Show resolved Hide resolved

ijjk reviewed Jun 17, 2020

View reviewed changes

packages/now-next/src/utils.ts Outdated Show resolved Hide resolved

ijjk reviewed Jun 17, 2020

View reviewed changes

packages/now-next/src/index.ts Outdated Show resolved Hide resolved

ijjk reviewed Jun 17, 2020

View reviewed changes

jeantil force-pushed the master branch from a933b83 to 25d589c Compare June 17, 2020 22:00

jeantil requested a review from TooTallNate as a code owner June 17, 2020 22:00

styfle reviewed Jun 17, 2020

View reviewed changes

packages/now-cli/test/dev/fixtures/monorepo-dynamic-paths/blog/next.config.js Outdated Show resolved Hide resolved

jeantil force-pushed the master branch 2 times, most recently from be12483 to e61f8a7 Compare June 17, 2020 22:18

jeantil force-pushed the master branch from e61f8a7 to ab51ee1 Compare June 17, 2020 22:22

styfle reviewed Jun 17, 2020

View reviewed changes

packages/now-cli/test/dev/fixtures/monorepo-dynamic-paths/blog/package.json Outdated Show resolved Hide resolved

jeantil force-pushed the master branch 5 times, most recently from a28fccc to a750c4a Compare June 18, 2020 11:42

jeantil force-pushed the master branch from a750c4a to 3cb439c Compare June 18, 2020 11:49

styfle added 3 commits June 18, 2020 19:11

Fix build.test.js

fa5d249

Add missing yarn.lock file

808a95a

Merge branch 'master' into master

04875dc

styfle approved these changes Jun 18, 2020

View reviewed changes

styfle requested a review from ijjk June 18, 2020 23:19

ijjk approved these changes Jun 18, 2020

View reviewed changes

styfle merged commit 200bf5e into vercel:master Jun 18, 2020

baukevanderlaan mentioned this pull request Jun 19, 2020

404 on dynamic routes in monorepo with now dev #4239

Closed

morgs32 mentioned this pull request Jul 16, 2020

Fix dynamic routes in multi-zone next deployment #4865

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[next] Fix nextjs routes in `vercel dev` #4510

[next] Fix nextjs routes in `vercel dev` #4510

jeantil commented May 27, 2020

jeantil commented May 27, 2020

ijjk commented May 29, 2020

jeantil commented May 30, 2020 •

edited

jeantil commented Jun 8, 2020

jeantil commented Jun 17, 2020

styfle commented Jun 17, 2020 •

edited

styfle commented Jun 17, 2020 •

edited

ijjk Jun 17, 2020

styfle Jun 17, 2020

jeantil Jun 17, 2020

ijjk Jun 17, 2020

jeantil Jun 17, 2020 •

edited

styfle Jun 17, 2020

jeantil commented Jun 17, 2020

jeantil commented Jun 18, 2020 •

edited

styfle left a comment •

edited

jeantil commented Jun 19, 2020

morgs32 commented Jul 11, 2020

jeantil commented Jul 13, 2020

morgs32 commented Jul 13, 2020 •

edited

jeantil commented Jul 13, 2020

morgs32 commented Jul 16, 2020

[next] Fix nextjs routes in vercel dev #4510

[next] Fix nextjs routes in vercel dev #4510

Conversation

jeantil commented May 27, 2020

jeantil commented May 27, 2020

ijjk commented May 29, 2020

jeantil commented May 30, 2020 • edited

jeantil commented Jun 8, 2020

jeantil commented Jun 17, 2020

styfle commented Jun 17, 2020 • edited

styfle commented Jun 17, 2020 • edited

ijjk Jun 17, 2020

Choose a reason for hiding this comment

styfle Jun 17, 2020

Choose a reason for hiding this comment

jeantil Jun 17, 2020

Choose a reason for hiding this comment

ijjk Jun 17, 2020

Choose a reason for hiding this comment

jeantil Jun 17, 2020 • edited

Choose a reason for hiding this comment

styfle Jun 17, 2020

Choose a reason for hiding this comment

jeantil commented Jun 17, 2020

jeantil commented Jun 18, 2020 • edited

styfle left a comment • edited

Choose a reason for hiding this comment

jeantil commented Jun 19, 2020

morgs32 commented Jul 11, 2020

jeantil commented Jul 13, 2020

morgs32 commented Jul 13, 2020 • edited

jeantil commented Jul 13, 2020

morgs32 commented Jul 16, 2020

[next] Fix nextjs routes in `vercel dev` #4510

[next] Fix nextjs routes in `vercel dev` #4510

jeantil commented May 30, 2020 •

edited

styfle commented Jun 17, 2020 •

edited

styfle commented Jun 17, 2020 •

edited

jeantil Jun 17, 2020 •

edited

jeantil commented Jun 18, 2020 •

edited

styfle left a comment •

edited

morgs32 commented Jul 13, 2020 •

edited