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

dependency-checks lint rule does not handle all workspace: version specifiers #29959

Closed
3 of 4 tasks
statop opened this issue Feb 10, 2025 · 3 comments
Closed
3 of 4 tasks
Assignees

Comments

@statop
Copy link

statop commented Feb 10, 2025

Current Behavior

dependency-checks lint rule currently will ignore the version on any dep that is workspace:*, it should just do a startswith workspace:

relative paths and some other things are valid in workspace:, for example workspace:../foo/bar/car

@meeroslav

Expected Behavior

dependency-checks lint rule supports all workspace version specifiers

GitHub Repo

No response

Steps to Reproduce

Nx Report

NX   Report complete - copy this into the issue template

Node           : 20.12.2
OS             : darwin-arm64
Native Target  : aarch64-macos
pnpm           : 9.15.4

nx                 : 19.8.14
@nx/js             : 19.8.14
@nx/jest           : 19.8.14
@nx/eslint         : 19.8.14
@nx/workspace      : 19.8.14
@nx/angular        : 19.8.14
@nx/devkit         : 19.8.14
@nx/eslint-plugin  : 19.8.14
@nx/nest           : 19.8.14
@nx/node           : 19.8.14
@nx/plugin         : 19.8.14
@nx/react          : 19.8.14
@nx/rollup         : 19.8.14
@nx/storybook      : 19.8.14
@nx/vite           : 19.8.14
@nx/web            : 19.8.14
@nx/webpack        : 19.8.14
typescript         : 5.5.4
---------------------------------------
Registered Plugins:
./tools/plugin/plugin.js
---------------------------------------
Community plugins:
@ngrx/effects         : 18.1.1
@ngrx/operators       : 18.1.1
@ngrx/store           : 18.1.1
@ngrx/store-devtools  : 18.1.1
@openadp/tools-plugin : 25.7.6
@storybook/angular    : 8.2.9
apollo-angular        : 7.0.2
ng-mocks              : 14.13.0
---------------------------------------
Local workspace plugins:
	 @openadp/tools-plugin

Failure Logs

Package Manager Version

No response

Operating System

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

Additional Information

No response

@ryanbas21
Copy link
Contributor

@statop does #29923 help resolve this? I wrote this a few days ago and added the other workspace specifiers to the if

@statop
Copy link
Author

statop commented Feb 11, 2025

@statop does #29923 help resolve this? I wrote this a few days ago and added the other workspace specifiers to the if

it doesn't look like it handles relative paths, like workspace:../foo/bar/cat

I would just check if it starts with workspace:

@ryanbas21

@ryanbas21
Copy link
Contributor

@statop does #29923 help resolve this? I wrote this a few days ago and added the other workspace specifiers to the if

it doesn't look like it handles relative paths, like workspace:../foo/bar/cat

I would just check if it starts with workspace:

@ryanbas21

Looks like @jaysoo added that to the PR. Nice callout, never really used file paths so didnt think about it :)

jaysoo added a commit that referenced this issue Feb 11, 2025
<!-- Please make sure you have read the submission guidelines before
posting an PR -->
<!--
https://github.com/nrwl/nx/blob/master/CONTRIBUTING.md#-submitting-a-pr
-->

<!-- Please make sure that your commit message follows our format -->
<!-- Example: `fix(nx): must begin with lowercase` -->

<!-- If this is a particularly complex change or feature addition, you
can request a dedicated Nx release for this pull request branch. Mention
someone from the Nx team or the `@nrwl/nx-pipelines-reviewers` and they
will confirm if the PR warrants its own release for testing purposes,
and generate it for you if appropriate. -->

## Current Behavior
Currently there doesn't seem to be support for pnpm catalogs in the
dependency check which throws an error.



## Expected Behavior
Catalog should be supported and at least not throw an error since its a
default rule in some generators.
I noticed that also that only `workspace:*` was implemented so I added
the other possible options.

https://pnpm.io/workspaces#publishing-workspace-packages

I copy and pasted a test twice to add tests for this.

I mean ideally I guess we would check the `pnpm-workspace` file and say
is this catalog defined, I don't think that's up my ally for this
purpose and I'm not sure if how its implemented does that for the
workspace protocols as well.

## Related Issue(s)
<!-- Please link the issue being fixed so it gets closed when this is
merged. -->

Fixes #
closed #29903, #29959


Gave it a shot, not sure if this is gonna be comprehensive enough. But
let me know! happy to try and get this right.

---------

Co-authored-by: Jack Hsu <jack.hsu@gmail.com>
@jaysoo jaysoo closed this as completed Feb 11, 2025
jaysoo added a commit that referenced this issue Feb 12, 2025
<!-- Please make sure you have read the submission guidelines before
posting an PR -->
<!--
https://github.com/nrwl/nx/blob/master/CONTRIBUTING.md#-submitting-a-pr
-->

<!-- Please make sure that your commit message follows our format -->
<!-- Example: `fix(nx): must begin with lowercase` -->

<!-- If this is a particularly complex change or feature addition, you
can request a dedicated Nx release for this pull request branch. Mention
someone from the Nx team or the `@nrwl/nx-pipelines-reviewers` and they
will confirm if the PR warrants its own release for testing purposes,
and generate it for you if appropriate. -->

## Current Behavior
Currently there doesn't seem to be support for pnpm catalogs in the
dependency check which throws an error.



## Expected Behavior
Catalog should be supported and at least not throw an error since its a
default rule in some generators.
I noticed that also that only `workspace:*` was implemented so I added
the other possible options.

https://pnpm.io/workspaces#publishing-workspace-packages

I copy and pasted a test twice to add tests for this.

I mean ideally I guess we would check the `pnpm-workspace` file and say
is this catalog defined, I don't think that's up my ally for this
purpose and I'm not sure if how its implemented does that for the
workspace protocols as well.

## Related Issue(s)
<!-- Please link the issue being fixed so it gets closed when this is
merged. -->

Fixes #
closed #29903, #29959


Gave it a shot, not sure if this is gonna be comprehensive enough. But
let me know! happy to try and get this right.

---------

Co-authored-by: Jack Hsu <jack.hsu@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants