Skip to content
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

Move dependencies to devDependencies (nx, ts-morph, tslib, @nx/devkit, angular-eslint/bundled-angular-compiler) #410

Closed
marcj opened this issue Jun 7, 2024 · 12 comments · Fixed by #581
Assignees
Labels
enhancement New feature or request

Comments

@marcj
Copy link

marcj commented Jun 7, 2024

In the ngxtension package we have lots of dependencies that will be installed in a project that has ngxtension as dependency: https://github.com/ngxtension/ngxtension-platform/blob/main/libs/ngxtension/package.json#L32

	"dependencies": {
		"@angular-eslint/bundled-angular-compiler": "^17.3.0",
		"@nx/devkit": "^18.0.0",
		"nx": "^18.0.0",
		"ts-morph": "^22.0.0",
		"tslib": "^2.3.0"
	},

None of these are actually required. Please remove them or move them to devDependencies.

In my current project I try to build the angular package via Docker based on Alpine, which fails because it thanks to ngextension it tries to pull in nx:

#12 39.01 npm error Error: Cannot find module '@nx/nx-linux-arm64-musl'
#12 39.01 npm error Require stack:
#12 39.01 npm error - /app/node_modules/nx/src/native/index.js
#12 39.01 npm error - /app/node_modules/nx/src/hasher/node-task-hasher-impl.js
#12 39.01 npm error - /app/node_modules/nx/src/hasher/task-hasher.js
#12 39.01 npm error - /app/node_modules/nx/src/hasher/hash-task.js
#12 39.01 npm error - /app/node_modules/nx/src/tasks-runner/run-command.js
#12 39.01 npm error - /app/node_modules/nx/src/nx-cloud/utilities/get-cloud-options.js
#12 39.01 npm error - /app/node_modules/nx/bin/post-install.js
$ npm ls nx
[email protected] /mycode
└─┬ [email protected]
  ├─┬ @nx/[email protected]
  │ └── [email protected] deduped
  └─┬ [email protected]
    └─┬ @nrwl/[email protected]
      └── [email protected] deduped

This prevents me from using ngextension, since I deploy on ARM machines. Also this bloats my node_modules and installs nx every time unnecessarily.

@jdegand
Copy link
Contributor

jdegand commented Jun 8, 2024

#304 mentioned that this was done for ng add.

This PR added functionality to allow devDependencies with the ng-add command.

#26 added the plugin, but it seems some defaults were never changed. See generators/convert-entry-point-to-project/schema.json.

@nartc
Copy link
Collaborator

nartc commented Jun 10, 2024

Last comment on #304

This seems to be a bigger problem than I initially realized. I have been experimenting with this over the last few days and I now assume that there is a structural problem with the package managers or the concept of migrations included within a library in general.

First of all, nx does not seem to be used by the schematics/migrations, neither for Angular CLI nor for Nx. @nx/devkit and ts-morph are in fact required for these schematics/migrations to work. In NPM it seems to be impossible to provide a package that can be partially used in production and development at the same time. At least when the dependencies differ.

I think there are a few unfavorable ways to handle such cases:

  • Add devopment dependencies to dependencies or peerDependencies and depend on tree shaking to get rid of them in production (as currently done for ngxtension)
  • Provide two separate packages, one for development and one for production (one could argue about separation of concerns here)
  • Bundle required development dependencies with bundleDependencies into the package (destroys all package management benefits)
  • Maybe it would be possible to use peerDependencies with an optional flag in their meta data and tell the library users to install these dependencies later on if needed (does not work with Angular schematics/Nx migrations)

I guess what would be really needed here is either peerDevDependencies or an additional dev flag in peerDependenciesMeta to provide transitive development dependencies.

Not sure what the best course of action would be in this case.

@nartc
Copy link
Collaborator

nartc commented Jun 10, 2024

Moving to devDependencies wouldn't work because ng-packagr strips them in the build output.

What we can do is to deprecate ng add and update getting started guide with installing the packages required.

# instead of 
# ng add ngxtension

# we will have
npm i ngxtension
npm i -D @nx/devkit

npx ng g ngxtension:init

For migrations (and ng update), we'll so have to have the consumers to install the required dependencies:

npm i -D ts-morph @angular-eslint/bundled-angular-compiler
npx ng update ngxtension

The DX does decrease a bit but we will not have bloated node_modules anymore and also unblock @marcj as well.

@jdegand
Copy link
Contributor

jdegand commented Jun 10, 2024

Have you looked into ng-packagr whitelisting? It is not well documented, but maybe this could be viable.
This issue might be worth looking at.

This PR gives an example for peerDependencies.

@nartc
Copy link
Collaborator

nartc commented Jun 11, 2024

@jdegand whitelistedNonPeerDependencies was deprecated in favor of allowedNonPeerDependencies due to BLM way back when; and we're already using allowedNonPeerDependencies on those exact dependencies because ng-packagr doesn't allow packaging with dependencies. devDependencies entry gets stripped away completely in the built package.json (in dist/libs... post build)

@ekrapfl
Copy link

ekrapfl commented Aug 6, 2024

Is there any progress on this issue? I am most interested in splitting into two packages, personally. That would at least solve the problem of someone wanting to use the awesome helpers like derivedAsync, but not being able to because installing ngxtension forces a ton of our dev dependencies to turn into prod dependencies.

@Kaemmelot
Copy link

Kaemmelot commented Aug 16, 2024

For everyone, who needs a quick solution:
I removed the unwanted dependencies temporarily by adding

  "overrides": {
    "ngxtension": {
      "nx": "npm:[email protected]",
      "@nx/devkit": "npm:[email protected]",
      "ts-morph": "npm:[email protected]"
    }
  }

to my package.json, which replaces the dependencies with empty packages.

@aaburov
Copy link

aaburov commented Mar 13, 2025

Is there any progress or update on this issue?

This is becoming a bigger nuisance for one of our applications. Since nx is a dependency in ngxtension, it is being security scanned as a production dependency. BlackDuck will tag certain nx's dependencies as vulnerabilities, failing our production scan.

@marcj
Copy link
Author

marcj commented Mar 13, 2025

guess we have to fork this one

@nartc
Copy link
Collaborator

nartc commented Mar 13, 2025

Hey folks, sorry for going hiatus on this issue. It's been on the back of my mind but I got busy with a new baby. Immediate plan is to publish ngxtension-plugin as a standalone lib and will update docs on all schematics/generators available as well as ng-add.

Expecting 4.6 or 4.7 over the weekend depending on whether we cut any release in between.

nartc added a commit that referenced this issue Mar 14, 2025
This publishes `ngxtension-plugin` as an npm package separately.

Closes #410

BREAKING CHANGE:
`ngxtension` no longer contains the generators/schematics.
- If you set up scripts / commands that use `ngxtension`
generators/schematics, please install `ngxtension-plugin` under
`devDependencies` and use `ngxtension-plugin` generators/schematics
instead.
- If you use `initGenerator` programmatically from `ngxtension`, please
update your import to `ngxtension-plugin`
@marcj
Copy link
Author

marcj commented Mar 14, 2025

congrats on the baby @nartc! Hope everything is well. please reach out if you need help with the code

@nartc nartc closed this as completed in 73c0ed7 Mar 14, 2025
@nartc
Copy link
Collaborator

nartc commented Mar 14, 2025

v5 has been released (to stay true to semver). Now we have 2 packages: ngxtension and ngxtension-plugin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants