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
Resolve to real path before checking for path inequality #17279
Changes from all commits
3caacd5
be7cc80
eac50c5
7e948d8
15a991d
05b04a6
5696cba
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,3 @@ | ||
export default function About() { | ||
return <div>About us</div> | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
export default function About2() { | ||
return <div>About 2</div> | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
export default function Day() { | ||
return <div>Hello Day</div> | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
import Link from 'next/link' | ||
export default function Home() { | ||
return ( | ||
<div> | ||
Hello World.{' '} | ||
<Link href="/about"> | ||
<a>About</a> | ||
</Link> | ||
</div> | ||
) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
/* eslint-env jest */ | ||
import { execSync } from 'child_process' | ||
import fs from 'fs-extra' | ||
import path from 'path' | ||
|
||
jest.setTimeout(100 * 1000) | ||
|
||
beforeAll(async () => { | ||
await clean() | ||
}) | ||
|
||
afterAll(async () => { | ||
await clean() | ||
}) | ||
|
||
test('pnpm installs', async () => { | ||
const pnpm = getStdout('yarn bin pnpm') | ||
exec(pnpm + ' init -y') | ||
exec(pnpm + ' add next react react-dom') | ||
await useLocalNextjs() | ||
// exec('pnpm install') | ||
}) | ||
|
||
test('nextjs builds with pnpm', () => { | ||
const pnpx = getStdout(`yarn bin pnpx`) | ||
exec(pnpx + ' next build') | ||
}) | ||
|
||
const exec = (cmd) => { | ||
return execSync(cmd, { | ||
env: process.env, | ||
shell: true, | ||
stdio: 'inherit', | ||
cwd: path.resolve(__dirname, '..'), | ||
}) | ||
} | ||
|
||
const getStdout = (cmd) => { | ||
return execSync(cmd, { | ||
env: process.env, | ||
shell: true, | ||
stdio: 'pipe', | ||
}) | ||
.toString() | ||
.trim() | ||
} | ||
|
||
async function useLocalNextjs() { | ||
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 function looks incorrect as it only considers packages/next but Next.js has a dependency of other packages in the monorepo as well which would not be copied, so this would break eventually. 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. Can you fix that in a follow-up PR as I'll merge this one. |
||
// installed nextjs | ||
const nextPath = path.dirname(require.resolve('next/package.json')) | ||
|
||
// repository root | ||
const root = path.dirname( | ||
require.resolve(path.resolve(process.cwd(), 'package.json')) | ||
) | ||
|
||
// local nextjs | ||
const currentNextPath = path.resolve(root, 'packages/next') | ||
|
||
// copy local nextjs to node_modules | ||
await fs.copy( | ||
path.resolve(currentNextPath, 'dist'), | ||
path.resolve(nextPath, 'dist') | ||
) | ||
|
||
console.log('copied local nextjs to node_modules') | ||
} | ||
|
||
async function clean() { | ||
// jest test cannot be found if a package.json exists in test directory | ||
await fs.remove(path.resolve(__dirname, '../package.json')) | ||
await fs.remove(path.resolve(__dirname, '../node_modules')) | ||
await fs.remove(path.resolve(__dirname, '../pnpm-lock.yaml')) | ||
} |
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.
Ideally, these pnpm tests should execute somewhere isolated from the monorepo's own
node_modules
. This helps prevent issues where a module is mistakenly imported from the monoreponode_modules
instead of pnpm'snode_modules
, leading to false test results.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.
There is no way to do that other than modifying the CI script, making it even more brittle
Furthermore, the test installs the dependencies before starting, the installed node_modules will always have precedence over the root node_modules
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.
That's fair.
I'm more worried about cases where a dependency is used by the Next.js app under test, but is missing from the pnpm
node_modules
tree.For example, if we forget to install
react
or another peer dependency, then it will instead be imported from the monoreponode_modules
.A more likely scenario is that a dependency is mistakenly omitted from the
next
package's dependencies list, e.g. #8344 (this example is for "next-server", but the same could apply to "next"). We want this to cause the pnpm integration test to fail, but if it's not isolated from the monoreponode_modules
, then the test will pass.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.
Is it possible to have this test create a temp folder outside of the next.js project tree, copy over package.json and pages and run the pnpm commands on that folder?
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.
yes i can do that