-
Notifications
You must be signed in to change notification settings - Fork 95
fix: incorrect path of middleware nft in setups using base directory #3262
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| /** @type {import('next').NextConfig} */ | ||
| const nextConfig = { | ||
| eslint: { | ||
| ignoreDuringBuilds: true, | ||
| }, | ||
| } | ||
|
|
||
| module.exports = nextConfig |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| { | ||
| "name": "pnpm-monorepo-base-proxy-app", | ||
| "version": "0.1.0", | ||
| "private": true, | ||
| "scripts": { | ||
| "build": "next build" | ||
| }, | ||
| "dependencies": { | ||
| "next": "latest", | ||
| "react": "^18.2.0", | ||
| "react-dom": "^18.2.0" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| export default function Home({ ssr }) { | ||
| return ( | ||
| <main> | ||
| <div data-testid="smoke">SSR: {ssr ? 'yes' : 'no'}</div> | ||
| </main> | ||
| ) | ||
| } | ||
|
|
||
| export const getServerSideProps = async () => { | ||
| return { | ||
| props: { | ||
| ssr: true, | ||
| }, | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| import type { NextRequest } from 'next/server' | ||
| import { NextResponse } from 'next/server' | ||
|
|
||
| export async function proxy(request: NextRequest) { | ||
| return NextResponse.json({ proxy: true }) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| [build] | ||
| base = "app" | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Most important part of this fixture is using |
||
| command = "pnpm run build" | ||
| publish = ".next" | ||
|
|
||
| [[plugins]] | ||
| package = "@netlify/plugin-nextjs" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| { | ||
| "name": "monorepo", | ||
| "private": true | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| packages: | ||
| - 'app' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,7 @@ import { exec } from 'node:child_process' | |
| import { existsSync } from 'node:fs' | ||
| import { appendFile, copyFile, mkdir, mkdtemp, readFile, rm } from 'node:fs/promises' | ||
| import { tmpdir } from 'node:os' | ||
| import { dirname, join } from 'node:path' | ||
| import { dirname, join, relative } from 'node:path' | ||
| import process from 'node:process' | ||
| import { fileURLToPath } from 'node:url' | ||
| import { cpus } from 'os' | ||
|
|
@@ -228,6 +228,14 @@ async function installRuntime( | |
| await rm(join(isolatedFixtureRoot, 'package-lock.json'), { force: true }) | ||
| } | ||
|
|
||
| let relativePathToPackage = relative( | ||
| join(isolatedFixtureRoot, siteRelDir), | ||
| join(isolatedFixtureRoot, packageName), | ||
| ) | ||
| if (!relativePathToPackage.startsWith('.')) { | ||
| relativePathToPackage = `./${relativePathToPackage}` | ||
| } | ||
|
|
||
| switch (packageManger) { | ||
| case 'npm': | ||
| command = `npm install --ignore-scripts --no-audit --legacy-peer-deps ${packageName} ${ | ||
|
|
@@ -248,7 +256,7 @@ async function installRuntime( | |
| env['YARN_ENABLE_SCRIPTS'] = 'false' | ||
| break | ||
| case 'pnpm': | ||
| command = `pnpm add file:${join(isolatedFixtureRoot, packageName)} ${ | ||
| command = `pnpm add file:${relativePathToPackage} ${ | ||
| workspaceRelPath ? `--filter ./${workspaceRelPath}` : '' | ||
| } --ignore-scripts` | ||
| break | ||
|
|
@@ -349,24 +357,27 @@ export async function deploySiteWithBuildbot( | |
| newZip.addLocalFolder(isolatedFixtureRoot, '', (entry) => { | ||
| if ( | ||
| // don't include node_modules / .git / publish dir in zip | ||
| entry.startsWith('node_modules') || | ||
| entry.startsWith('.git') || | ||
| entry.includes('node_modules') || | ||
| entry.includes('.git') || | ||
|
Comment on lines
+360
to
+361
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. new fixture has nested directory with |
||
| entry.startsWith(publishDirectory) | ||
| ) { | ||
| return false | ||
| } | ||
| return true | ||
| }) | ||
|
|
||
| const result = await fetch(`https://api.netlify.com/api/v1/sites/${siteId}/builds`, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/zip', | ||
| Authorization: `Bearer ${process.env.NETLIFY_AUTH_TOKEN}`, | ||
| const result = await fetch( | ||
| `https://api.netlify.com/api/v1/sites/${siteId}/builds?clear_cache=true`, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added However with this test we use shared |
||
| { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/zip', | ||
| Authorization: `Bearer ${process.env.NETLIFY_AUTH_TOKEN}`, | ||
| }, | ||
| // @ts-expect-error sigh, it works | ||
| body: newZip.toBuffer(), | ||
| }, | ||
| // @ts-expect-error sigh, it works | ||
| body: newZip.toBuffer(), | ||
| }) | ||
| ) | ||
| const { deploy_id } = await result.json() | ||
|
|
||
| let didRunOnBuildStartCallback = false | ||
|
|
@@ -636,6 +647,16 @@ export const fixtureFactories = { | |
| publishDirectory: 'apps/site/.next', | ||
| smoke: true, | ||
| }), | ||
| pnpmMonorepoBaseProxy: () => | ||
| createE2EFixture('pnpm-monorepo-base-proxy', { | ||
| buildCommand: 'pnpm run build', | ||
| generateNetlifyToml: false, | ||
| packageManger: 'pnpm', | ||
| publishDirectory: '.next', | ||
| runtimeInstallationPath: 'app', | ||
| smoke: true, | ||
| useBuildbot: true, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is annoying because we don't see build failure logs when using buildbot, but I couldn't replicate the exact conditions with CLI deploys. https://app.netlify.com/projects/next-runtime-testing/deploys/691db94c23e26600e071528e (this might get automatically deleted soon!) is example build reproducing kind of errors without any runtime code changes (1st commit) that is fixed by runtime changes (2nd commit) |
||
| }), | ||
| dynamicCms: () => createE2EFixture('dynamic-cms'), | ||
| after: () => createE2EFixture('after'), | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is second attempt at fixing this path (previous one was in #3211 ).
This time using
publishDiras it already should point to absolute path to.next.We already use
publishDirin middleware handling to find relevant manifest files:opennextjs-netlify/src/build/plugin-context.ts
Lines 262 to 286 in 6451ec3
and since those work (we wouldn't get to code path trying to read
.nftfile if they didn't), it's good idea to just reuse this method.Some explanation why current method is not working in some cases:
In case of monorepos that are configured to use
basedirectory instead ofpackagePath, the CWD will be workspace and not root of monorepo. Thectx.distDirwill be path from monorepo root to.nextdirectory (so will be prefixed withbasedirectory). Both combined means thatbasedirectory is repeated in produced final path. Using example with repo that hasappworkspace and Netlify setup usingbasedirectory set toapp:Note: with last fix attempt we did add smoke fixture using packagePath. In this PR I add another smoke fixture replicating monorepo setup using base directory instead of packagePath. Both are passing with the changes