milliHQ / terraform-aws-next-js

Terraform module for building and deploying Next.js apps to AWS. Supports SSR (Lambda), Static (S3) and API (Lambda) pages.
https://registry.terraform.io/modules/milliHQ/next-js/aws
Apache License 2.0
1.47k stars 151 forks source link

npm 7 workspaces support #135

Open thomasdashney opened 3 years ago

thomasdashney commented 3 years ago

Hi, thanks for your great work on this library 🙂

I'm using npm 7 workspaces for my monorepo.

e.g. repo structure:

packages/
  my-next-app/
    package.json
  shared/
    package.json
package.json
package-lock.json

The workspaces are configured in the root package.json ("workspaces": ["./packages/*])

Because of how npm workspaces works, I need to run npm install from the root folder. This creates the symlinks correctly.

When I run tf-next from the packages/my-next-app/ folder, it thinks it's a yarn package, and runs yarn install, which then breaks during the build. I tried to force it to use npm by adding an empty package-lock.json to packages/my-next-app/package-lock.json. This did run npm install (instead of yarn), but npm install was not running from the root, causing the build to break again.

IMO, it would be convenient to have the option to skip package installation when running tf-next (so I can be free to run npm install as needed for my project beforehand). Please let me know what you think, and thanks in advance!

ofhouse commented 3 years ago

Hi, yep since npm 7 is stable this we should support this out of the box 👍

The functionality comes from an external package developed by Vercel that already supports npm 7. So chances are not too bad that we only need to bump the version of that package to make this work. Will take a look soon.

The install step can already be skipped with the --skipDownload flag:

npm i
tf-next build --skipDownload

Unfortunately it is called skipDownload instead of skipInstall for historical reasons, so we should also provide an alias for that flag with an appropriate name (And add a better documentation to it) 😅

thomasdashney commented 3 years ago

Oh interesting - I tried using the --skipDownload flag, but it still runs yarn install for me 🤔

I created a fork and made a quick tweak to try skipping the installation step: https://github.com/thomasdashney/terraform-aws-next-js/commit/da24b40ac34065a4822269e82f1a49234b8f4c7a

However, now I'm getting an error:

NowBuildError: No Next.js version could be detected in your project. Make sure "next" is installed in "dependencies" or "devDependencies"

I'm not familiar with the Next.js build system so this could be an easy fix, but mentioning this here incase the info helps!

ofhouse commented 3 years ago

Tested a npm workspace on Vercel and they always fallback to yarn for monorepo setups. The code that determines if yarn or npm should be used for install can be found here: https://github.com/vercel/vercel/blob/2d70c6c811789408d7981182a146f6d1f61a44e4/packages/build-utils/src/fs/run-user-scripts.ts#L281

Since we also rely on the code it would make sense to start an issue over at the vercel repo first to fix this upstream.

khuezy commented 2 years ago

@ofhouse Can build-utils be updated to at least 2.12.1? It contains the cliType fix. https://github.com/vercel/vercel/pull/6559

ofhouse commented 2 years ago

@khuezy Sorry for the late reply, will give it a try 👍

ofhouse commented 2 years ago

An upgrade to the build-utils package is now released in v0.11.5.