fix: resolve site dependency version from packagePath if defined #6650
Conversation
pieh
force-pushed
the
fix/resolve-site-dependency-version-from-package-path-first
branch
from
4c22d9e to
9d1f119
Compare
pieh
force-pushed
the
fix/resolve-site-dependency-version-from-package-path-first
branch
from
9d1f119 to
579c160
Compare
|
This pull request adds or modifies JavaScript ( |
pieh
changed the title
| const runWithApiMock = async function ( | ||
| t, | ||
| fixtureName, | ||
| { testPlugin, response = getPluginsList(testPlugin), ...flags } = {}, | ||
| status = 200, | ||
| ) { | ||
| await t.snapshot(await runWithApiMockAndGetNormalizedOutput(fixtureName, { testPlugin, response, ...flags }, status)) | ||
| } | ||
|
|
There was a problem hiding this comment.
All existing tests in this suite use snapshots which is not something I wanted to continue and wanted something a bit more concrete, so I moved most of this to another helper that just returns normalized output instead of snapshotting it.
Would be nice to convert existing tests to explicit assertions, but I don't have bandwidth for that right now
| const packageJsonPath = await resolvePath(`${dependencyName}/package.json`, buildDir) | ||
| const packageJsonPath = await resolvePath( | ||
| `${dependencyName}/package.json`, | ||
| packagePath ? join(buildDir, packagePath) : buildDir, |
There was a problem hiding this comment.
nit - we seem to mostly be using join(buildDir, packagePath || '') or packagePath ?? ''
| @@ -81,7 +83,7 @@ const siteDependencyTest = async function ({ | |||
|
|
|||
| try { | |||
| // if this is a range we need to get the exact version | |||
| const packageJsonPath = await resolvePath(`${dependencyName}/package.json`, buildDir) | |||
| const packageJsonPath = await resolvePath(`${dependencyName}/package.json`, join(buildDir, packagePath || '')) | |||
There was a problem hiding this comment.
Very similar to fix in #6105, where other part of the system was not taking packagePath into consideration
Do note that if dependency is in node_modules in root of monorepo - we still will be able to resolve, because require.resolve is used and not direct FS check. This is also verified with added test case for hoisted node_modules still passing
…laces in codebase
pieh
force-pushed
the
fix/resolve-site-dependency-version-from-package-path-first
branch
from
fdeca6f to
596e0b2
Compare
pieh
deleted the
fix/resolve-site-dependency-version-from-package-path-first
branch
🎉 Thanks for submitting a pull request! 🎉
Summary
Ref: https://netlify.slack.com/archives/C01TKAEBP3Q/p1756940014614849
To easier follow added fixtures - recommend checking them in "file explorer" instead of diff and notice where
node_modulesare situated in those 2 fixtures:First commit with failing test that this PR is looking to address - https://github.com/netlify/build/actions/runs/17465897278/job/49601431168?pr=6650#step:12:313
Second commit with the fix
For us to review and ship your PR efficiently, please perform the following steps:
we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or
something that`s on fire 🔥 (e.g. incident related), you can skip this step.
your code follows our style guide and passes our tests.
A picture of a cute animal (not mandatory, but encouraged)