ngxtension / ngxtension-platform

Utilities for Angular
https://ngxtension.netlify.app/
MIT License
529 stars 78 forks source link

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

Open marcj opened 4 weeks ago

marcj commented 4 weeks ago

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
provision@1.0.0 /mycode
└─┬ ngxtension@2.3.2
  ├─┬ @nx/devkit@17.3.2
  │ └── nx@17.3.2 deduped
  └─┬ nx@17.3.2
    └─┬ @nrwl/tao@17.3.2
      └── nx@17.3.2 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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

@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)