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

Manually adding a devDependency to a package.json file no longer excludes it from dependencies #17758

Closed
1 of 4 tasks
ajwootto opened this issue Jun 23, 2023 · 7 comments · Fixed by #17802
Closed
1 of 4 tasks

Comments

@ajwootto
Copy link
Contributor

Current Behavior

In previous versions of Nx, adding a devDependencies entry for another package in your repo (say something only used by tests) would tell Nx to not add it to the "dependencies" section of the generated package.json for that package.

Now it appears in both "dependencies" and "devDependencies". This breaks the ability to correctly publish packages with Nx.

Expected Behavior

It should work the same as before

This issue is compounded by the fact that most of the Nx build executors are still adding dependencies of dependencies to the output package.json

Here is a helpful list of all the related issues compiled by @JosefBredereck
#10227 (comment)

GitHub Repo

https://github.com/devcyclehq/js-sdks

Steps to Reproduce

  1. clone that repo
  2. run nx build nodejs
  3. inspect `dist/sdk/nodejs/package.json
  4. Observe that there is an entry for @devcycle/bucketing-test-data in the "dependencies" section of package.json as well as devDependencies. It should only be listed in devDependencies

Nx Report

>  NX   Report complete - copy this into the issue template

   Node   : 18.16.0
   OS     : darwin arm64
   yarn   : 3.6.0
   Hasher : Native

   nx                 : 16.3.2
   lerna              : 5.6.2
   @nx/js             : 16.3.2
   @nx/jest           : 16.3.2
   @nx/linter         : 16.3.2
   @nx/workspace      : 16.3.2
   @nx/cypress        : 16.3.2
   @nx/detox          : 16.3.2
   @nx/devkit         : 16.3.2
   @nx/esbuild        : 16.3.2
   @nx/eslint-plugin  : 16.3.2
   @nx/next           : 16.3.2
   @nx/node           : 16.3.2
   @nx/react          : 16.3.2
   @nx/react-native   : 16.3.2
   @nx/rollup         : 16.3.2
   @nrwl/tao          : 16.3.2
   @nx/web            : 16.3.2
   @nx/webpack        : 16.3.2
   typescript         : 5.0.4
   ---------------------------------------
   Local workspace plugins:
   	 @devcycle/bucketing-assembly-script
   	 @devcycle/openfeature-nodejs-provider
   	 @devcycle/bucketing-test-data
   	 @devcycle/bucketing
   	 @devcycle/devcycle-react-native-sdk
   	 @devcycle/types
   	 @devcycle/nodejs-server-sdk
   	 @devcycle/devcycle-react-sdk
   	 @devcycle/devcycle-js-sdk

Failure Logs

No response

Operating System

  • macOS
  • Linux
  • Windows
  • Other (Please specify)

Additional Information

this is mission-critical behaviour of Nx IMO, it has caused an incident on our side due to publishing packages that cannot be installed by end-users.

@ajwootto
Copy link
Contributor Author

ajwootto commented Jun 26, 2023

Did a little digging and I think the problem was introduced in this PR
#13968

when the js executor stopped using the updateBuildableProjectPackageJsonDependencies method from the workspaces package and replaced it with a new implementation that did not have equivalent behaviour.

You can still see the old codepath for removing the devDependencies present here:

// If an npm dep is part of the workspace devDependencies, do not include it the library

@ndcunningham
Copy link
Contributor

I understand that the current approach may not be ideal, and I appreciate your feedback. Rest assured, we are actively working on enhancing the handling of package.json to make it more robust and flexible to accommodate various use cases.

In the meantime, I'm glad that you found a workaround. It serves as a temporary solution until we implement the improvements.

I will keep this issue open so that we can refer back to it during our ongoing discussions and development process. Feel free to reach out if you have any further questions or concerns.

@ajwootto
Copy link
Contributor Author

@ndcunningham appreciate the response

I'm glad to hear you're making improvements in the area, but would it also not be possible to get in a quick fix to restore this behaviour in the meantime? It's technically a breaking change that I believe was introduced accidentally, and my "workaround" is basically to apply a patch to the js package that fixes the bug. I know it's maybe a slightly niche behaviour, but for us it's basically a "we can no longer use Nx unmodified to publish our packages" type of issue.

@scott-avery
Copy link

@ndcunningham It's indeed a breaking changes for publish libraries for latest nx(v16.4.3). Hope It could be fixed asap.

That PR @ajwootto mentioned was introduced long time ago. We can not rollback to an older version to bypass the problem.

@jaknas
Copy link

jaknas commented Aug 8, 2023

YMMV but it setting external: "all" seems to resembling the old behavior.

 "targets": {
        "build": {
            "executor": "@nx/rollup:rollup",
            "options": {
                 ...
                "external": "all",

@ajwootto
Copy link
Contributor Author

FYI until this is fixed you can try applying the fix I have in this PR:
https://github.com/nrwl/nx/pull/17802/files

using something like yarn patch. You'll have to adapt it to the plain JS output that's distributed in the Nx packages.

It also means you'll be stuck at that version of Nx (like we are) unless you want to keep applying the patch post-update. For us we just aren't going to update Nx again until this is fixed.

@github-actions
Copy link

github-actions bot commented Oct 7, 2023

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants