generatePackageJson on nx 15 generates the the dependencies and versions differently than 14 #12675
Comments
|
Also experiencing this. Ours looks like it is pulling in every dep, theres like 495 different packages in the dists package.json, we only have 341 in our root package.json |
|
I'm also experiencing this issue. The number of Before 15 After 15 |
|
This should have the same root cause as #12660 |
FrozenPandaz
added
scope: devkit
scope: core
|
Hi @JoA-Mo, Your docker build process is copying the root This should no longer be needed as the generated |
Question about this though. Our lock file has all the versions that the apps were built and tested with. But when we copy it in and use it with the generated package JSON the frozen lockfile flag causes it to fail. You said we shouldn't be copying in the lockfile anymore as everything is pinned but aren't we introducing risk because the generate package JSON is being generated with different versions from the lockfile (thus we would be running In production different versions than it was built/tested with). |
|
@charsleysa - for docker images that might work, but there are also additional consequences related to using generatePackageJson when building and publishing an npm package. It will inflate the number of direct dependencies that your package depends on as shown in NPM. this info: |
|
Doing the below code in the Dockerfile of our server and now getting I agree with the other commenters points, won't this change introduce additional risk since the versions we develop/test with locally can differ from what the docker image builds with? Reverting back to v14 until more clarification on this. |
|
We do the same process as @enchorb with the frozen lockfile flag, if the generate package json were truely correct and we didn't need out lockfile then we should be able to have our lockfile there and be able to use --frozen-lockfile successfully. |
|
@JoA-MoS for publishing packages, one option could be to use this workaround posted in another issue #12660 (comment) though I do think that the previous functionality should be made available using a config option for those who are publishing packages. @enchorb @yharaskrik the generated |
|
@charsleysa - thanks for providing some options, I can delay the update to 15, but wanted to report this to the nx team as I don't think this is intended behavior. |
|
Ok so this is really weird, curious if anyone has some input here, no idea if it's related to Nx or not but Nx was the only part that I updated here in this PR. When I build my docker container now (I am not copying Looks like a memory issue, again, the only thing that change between this build and a previous one is that I updated Nx and the generated package json changed. Our other apps successfully build it's just this one fails. The one failing (now) has 702 ish packages in the package.json, the one that succeeds only has like 65. This was working before so I don't realllly want to crank the resources up on CI to get past this. Any docker experts around here that can point me in the direction of what I can do so the new way Nx generates the package.json will work for us? |
|
Not sure if the container will run after this (i don't see why not) but this little script that is ran after the build seems to have solved my memory issue woes. const { readFileSync, writeFileSync } = require('fs');
const { join } = require('path');
/**
* @description Trim dependencies from the generated built package.json so it's only the first level of
* dependencies and not everything.
*
* @type {any}
*/
const packageJson = JSON.parse(readFileSync(join(__dirname, '../package.json'), { encoding: 'utf8' }));
const distPackageJson = JSON.parse(
readFileSync(join(__dirname, '../dist/apps/<app>/package.json'), { encoding: 'utf8' })
);
Object.keys(distPackageJson['dependencies']).forEach((key) => {
if (!packageJson['dependencies'][key]) {
distPackageJson['dependencies'][key] = undefined;
}
});
writeFileSync(join(__dirname, '../dist/apps/<app>/package.json'), JSON.stringify(distPackageJson)); |
|
@yharaskrik - I am curious do you get this error outside of the container? IE copy the |
I can try it but I doubt it. I have a brand new MacBook pro M2 with a ton of RAM. |
Installs fine on my local machine. |
Something isn't working right. Our root package.json defines Is there any plans to revert this generate package json change? If not I will need to write some more scripts to fix this ourselves as I don't think I can trust the generated one. |
|
Any news on this? After updating to nx 15 all my express apps using When comparing the old output and the new, It looks like v15 is including all the peer dependencies as top level dependencies instead. So I'm getting a lot of: |
|
Hi, There migh be other packages, but in this case the application is using a function that was deployed in uuid version 8.3.0, so it is cashing. Interim solution: added Hope this helps. |
Yes, I was also getting incorrect versions of packages used. for everyone here, after you run build with the flag you can use this flag to fix your package.json const { readFileSync, writeFileSync } = require('fs');
const { join } = require('path');
const app = process.argv[2];
const packageJson = JSON.parse(readFileSync(join(__dirname, '../package.json'), { encoding: 'utf8' }));
const distPackageJson = JSON.parse(
readFileSync(join(__dirname, `../dist/apps/${app}/package.json`), { encoding: 'utf8' })
);
Object.keys(distPackageJson['dependencies']).forEach((key) => {
const version = packageJson['dependencies'][key];
if (!version) {
distPackageJson['dependencies'][key] = undefined;
} else {
distPackageJson['dependencies'][key] = version;
}
});
writeFileSync(join(__dirname, `../dist/apps/${app}/package.json`), JSON.stringify(distPackageJson));then run it with |
|
Had the same thing happen with a version of axios. It might be adding packages NX itself is using. |
|
@FrozenPandaz @meeroslav is this something that is gonna be fixed/reverted to the 14.x behaviour or are you keeping it? I just think we all need some feedback because currently there is no valid solution to this. Thanks! If I am looking at it correctly this change in behaviour was made in #12185 |
JoA-MoS
changed the title
|
Hey @capJavert, We will not revert the code to v14, but we are working on the solution for this case. In v14 we were keeping the version ranges as they were in package.json, which oftentimes caused unexpected errors when versions changed. The v15 uses fixed versions. The additional dependencies issue is something we will investigate and will get back to you as soon as possible. |
|
Hey @meeroslav thanks for your reply. One thing I will note is that there is one other issue more than the additional deps issue. There are cases where the version in the generated package json is wrong compared to the root package.json (and thus the lock file). For the one that happened to me |
|
Hmm... code looks fine. Is there some minimal repo where I can debug this? |
|
I was able to make the issue recur locally after updating axios to v 1.1.3. JoA-MoS/garage#85 Root package.json Generated package.json nxdeps.json node_modules/axios/package.json Is there a bug in how |
|
I am actually looking at https://github.com/nrwl/nx/blob/master/packages/nx/src/project-graph/build-project-graph.spec.ts to see if I can add this test case but not that familiar with the code so I am getting oriented. Let me know if you have decided this is not the correct path. |
|
@meeroslav - Here is a very simple repro library. I added the latest chalk 5.1.2 and would expect that to be included in the package.json when generated but the version is set to 2.4.2. https://github.com/JoA-MoS/repro-nx-generate-package-json pull the repo and run Then check |
|
@JoA-MoS I've checked out your repro and confirmed the issue. @meeroslav I've debugged the issue and traced it down to this function https://github.com/nrwl/nx/blob/master/packages/nx/src/utils/target-project-locator.ts#L132 From the context of the repro, what looks like is happening is that there are multiple versions of A solution I came up with (which may not be correct) is to add the with |
|
Not sure if this is right either but instead of adding the root version
(since the hoisted version may be different) instead what about when
deciding what version you read it from the package.json of the package. So
in that function you mentioned you do something like (sorry on my phone)
readJson(join(cwd, node_modules, packageName, "package.json")).version
…On Fri., Oct. 28, 2022, 8:52 p.m. Stefan Charsley, ***@***.***> wrote:
@JoA-MoS <https://github.com/JoA-MoS> I've checked out your repro and
confirmed the issue.
@meeroslav <https://github.com/meeroslav> I've debugged the issue and
traced it down to this function
https://github.com/nrwl/nx/blob/master/packages/nx/src/utils/target-project-locator.ts#L132
From the context of the repro, what looks like is happening is that there
are multiple versions of chalk installed (due to different used by
dependencies) and the graph contains all those versions (since it reads the
entire lock file) but the listed dependency version does not come first in
the list. The findNpmPackage function doesn't distinguish package
versions so it returns the first package where the name matches regardless
of version (which happens to be ***@***.***).
A solution I came up with (which may not be correct) is to add the
rootVersion property to the data during mapLockFileDataToPartialGraph
https://github.com/nrwl/nx/blob/master/packages/nx/src/utils/lock-file/lock-file.ts#L133
and updating the findNpmPackage function by replacing
const pkg = this.npmProjects.find(
(pkg) =>
npmImport === pkg.data.packageName ||
npmImport.startsWith(`${pkg.data.packageName}/`)
);
with
const pkg = this.npmProjects.find(
(pkg) =>
(npmImport === pkg.data.packageName ||
npmImport.startsWith(`${pkg.data.packageName}/`)) &&
pkg.data.rootVersion === true
) ?? this.npmProjects.find(
(pkg) =>
npmImport === pkg.data.packageName ||
npmImport.startsWith(`${pkg.data.packageName}/`)
);
—
Reply to this email directly, view it on GitHub
<#12675 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACIHZIWYLDD3YCJAZGHUF7TWFSNQZANCNFSM6AAAAAARHQPANM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Hoisting occurs in npm, pnpm, yarn, and lerna workspaces where there are multiple package.json for each project. In this situation the dependency version in the package.json of the project if defined should be used. If it is not defined it should take the root package.json. All of these tools already have a way of figuring this out so I think it would be ideal to utilize the package manager resolution of the dependency. Now nx without workspaces nx is responsible for figuring out this and I think it should just rely on the closest package.json dependency version. If it is defined in the project's package.json use that version. If it is defined at the root use that version. If you use npm workspaces your package lock looks like following The difficulty is that with nx without package manager workspaces this isn't defined like this. My thought is that the generated package.json should use the package version defined in the package.json closest to the app/lib that uses it. If it is not defined in the project folder then move up to the root package.json. (There is the consideration of nested projects (which I don't think people should use) but the algorithm is still the same. If the package is not defined move up to the next package.json until you get to the root package.json. |
Correction, if we don't have npm/yarn workspaces the only version of a dependency you can have is the top root dependency, if you were to specify a different version of a package in a projects package.json it would never be installed. The other method for multiple versions is npm aliases which sorts itself out. npm alias |
|
@meeroslav, @FrozenPandaz - Can you explain why the generartePackageJson process was changed? Was it intentional or a side effect of something else? What is the process for reopening this issue? do we reopen this issue or create a new one? |
|
Still experiencing this - packages that are supposed to be lenient (from the root package.json |
|
@JoA-MoS first of all, thank you for the repro. This is super helpful and allows me to pinpoint the source of the problem (kudos to @charsleysa for doing most of the work in locating it).
How we treat external dependencies (this also includes package json generation) was semi-broken for a very long time. The parsing of lock files finally allowed us to have full insight into which packages are actually used. This change helped us fix several issues that couldn't be fixed otherwise.
No need to reopen it. I will create another fix in a bit and link it to this issue. |
|
@charsleysa the solution is a bit more complicated than that but you were on the right track. Detecting which of the versions is the root one is the key. I just need to make sure the solution doesn't impact the performance. |
TLDR;This is by design. We intentionally want to restrict generated package.json dependencies to those that we have in the lock file since that is the only version we can guarantee to work. Long versionThere are reasons why lock files exist:
Reasons 2-4 (most importantly 3) are the reasons why we want to have a fixed version in the generated To achieve this, each In all the other scenarios we should run The only reason we didn't have it done this way so far, was that we didn't have lock file parsing in place so we couldn't tell which versions you actually have. We will soon add lock file pruning, so each generated package.json will be accompanied by a matching lock file. |
|
@meeroslav - thank you so much for your work on this, it is a wonderful tool. I have a different understanding about how package managers and lock files work but haven't found it documented so I am wondering if there has been conversations about this with the npm / yarn / pnpm teams and confirmed that using the whole lock file with a subset package.json is not supported?
2 - 4 above were solved in version 9 (I think) through 14's implementation of generatePackageJson by using the following process.
CONFIRM: Run Usually the above is done in a Dockerfile Maybe, you have confirmed that the above process is an unsupported process, I am wondering if the complexity of processing the different package managers lock files, in the different configurations (root only, workspaces), and evaluating the correct version to include in the generated package.json and pruned lock file adds value to the above process? In my opinion, looking at the justification for this added functionality all the items were already solved in v14 generate package json algorithm it just wasn't documented well. If it would help I can write up a full recipe for publishing apps / docker images / lambda with the exact dependencies using the root lock file. |
|
Also, the new generated pacakge json may have adverse impacts for publishable npm packages as the range specified is important and needs to be included in the generated package json. This also impacts any publishable package using the |
This is more of a workaround than a proper solution. That's why we want to offer automated one. Lock file generation should be available in few weeks.
As mentioned earlier, parsing and pruning the lock files was absolutely must since it was blocking several issues and features. So it this was just a side win.
For publishable packages we suggest creating manual package.json instead of using generated one, because it gives you the freedom to specify ranges as you see fit. For example my library could support |
|
Scratch that. |
|
@yharaskrik there were several fixes added recently for this logic and not all of them are rolled out yet. This is included in the latest version:
This will be included in the upcoming version:
Hopefully, that should cover it. |
|
Has this been released yet? Still getting the |
|
@enchorb The error you see is not related to the included fixes, but rather to the upcoming implementation of lock file pruning. Unfortunately, you have to be a bit more patient with that. |
|
Hi, I've already moved on to the new version which fixed the issue with dependency version but I am now seeing an issue with using an aliased npm package. I'm not sure if this is related to this fix or a separate issue. When I have a package installed using Is the fix to look for the
|
|
Thanks @Nigelli, can you please open a separate issue and tag me and this issue so I can follow up. |
|
@meeroslav - Something occurred to me a while back but just wanted to add it to the thread. In regards to this comment.
I believe specifying the range beyond the version used to build the package is what the peerDependencies section of the package.json is used for. |
|
Usually yes, but not exclusively. Peer only means: "my package needs this to work, but I'm expecting other dependencies to have already installed it and will not install it myself." |
|
This issue has been closed for more than 30 days. If this issue is still occuring, please open a new issue with more recent context. |
Current Behavior
npm ciof a a copied apps dist directory breaks after update to nx 15 because the root package-lock.json dependency version does not match the generated package.json dependency version.With nx 14 when using the
"executor": "@nrwl/node:webpack",with"generatePackageJson": truethe version of the dependency in the generated package.json matched the root package.json including the range.With nx 15 and the same config the the generated package.json dependency appears to be the evaluated exact version without the range.
before nx 15
after nx 15 update
Expected Behavior
the versions in the generated package.json should be exactly the same including range of the root package.json
Steps to Reproduce
Working Branch using nx 14 - https://github.com/JoA-MoS/garage/
Bad branch PR using nx 15 - JoA-MoS/garage#81
Run build on both and compare generated package.json
Failure Logs
Environment
The text was updated successfully, but these errors were encountered: