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

Add support for workspace projects #67

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

slheavner
Copy link

npm ls for workspace projects include the "root" project, resulting in ropm including the workspace project as a dependency of itself, and adding postfixes (_v1, etc.) to its actual dependencies

How:

  • use --json for npm ls instead of --parseable
  • iterate items in the json dependency try until we find the matching name field
  • use that item as the "root" dependency graph
  • flatten the json to match the original file path format

@slheavner slheavner force-pushed the feature/workspace-support branch 2 times, most recently from 1c55545 to ac6d7dc Compare January 18, 2023 18:02
@TwitchBronBron
Copy link
Member

One of the unit tests is failing in the CI, so can you take a look at that?

Also, does this change need to account for aliases (when we install some package with a different name)?

@slheavner
Copy link
Author

slheavner commented Jan 19, 2023

One of the unit tests is failing in the CI, so can you take a look at that?

Also, does this change need to account for aliases (when we install some package with a different name)?

I think I got caught by mac's case-insensitive file system 🤦

The name comparison here doesn't touch the dependencies, it's only comparing the "host" package name to make sure it is used as the root dependency in the graph, so those aliases shouldn't apply

@TwitchBronBron
Copy link
Member

Looks like it's still failing.

@slheavner slheavner force-pushed the feature/workspace-support branch from fd46d32 to 5898388 Compare January 22, 2024 20:48
@TwitchBronBron
Copy link
Member

oh wow, I completely forgot about this! I'll try and give it a good review this week. Sorry about that!

@slheavner
Copy link
Author

no worries, revisiting.

There the problem with CI is the npm version tagged there vs what I was testing against. I need to dig into the differences and find what's causing it.

- `npm ls` always lists dependencies from the root project, even if
  running from a workspace project. This changes uses the `--json` and
`--long` options in order to only parse dependencies from the current
project tree
- note: node_modules loctiaon changes significantly between npm versions
@slheavner slheavner force-pushed the feature/workspace-support branch from 5898388 to 741ad53 Compare January 23, 2024 18:11
@slheavner
Copy link
Author

workflow passed on my fork, should be good to go

@TwitchBronBron
Copy link
Member

I'm so sorry this fell through the cracks for so long. Do you mind taking a look at the merge conflicts and letting us know if this is still something that is needed?

Also, I'm not entirely familiar with "workspace projects", so do you mind giving a quick overview of that that actually means? Once I have a better understanding of what this solves, I'm happy to merge it and cut a release.

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

Successfully merging this pull request may close these issues.

2 participants