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

fix(js): filter project dependencies when calculating topological ordering #26491

Merged
merged 3 commits into from
Jun 11, 2024

Conversation

mpsanchis
Copy link
Contributor

Topological ordering of projects in the graph, used when calculating versions with @nx/js:release-version, has a bug. Projects are not sorted topologically correctly, leading to unexpected versioning behaviour.

Fixes #26490

…ering

For each project being sorted, only its dependent projects must be processed.
@mpsanchis mpsanchis requested a review from a team as a code owner June 10, 2024 15:58
@mpsanchis mpsanchis requested a review from mandarini June 10, 2024 15:58
Copy link

vercel bot commented Jun 10, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated (UTC)
nx-dev ⬜️ Ignored (Inspect) Visit Preview Jun 11, 2024 0:29am

Copy link
Collaborator

@JamesHenry JamesHenry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @mpsanchis, please can you add a unit test in packages/js/src/generators/release-version/utils/sort-projects-topologically.spec.ts that demonstrates the behavior you are addressing here (i.e. it should fail before your change, and pass afterwards)

@JamesHenry JamesHenry self-assigned this Jun 10, 2024
@JamesHenry JamesHenry removed the request for review from mandarini June 10, 2024 16:55
@mpsanchis
Copy link
Contributor Author

Hi @JamesHenry
Thanks for the quick reply. I have added two "interesting" tests. One of which (should return [C,B,A] if A depends on B and B depends on C) failed before the PR. I have also grouped the test cases.
Please let me know if you guys have other ways of doing things 😄

@JamesHenry
Copy link
Collaborator

JamesHenry commented Jun 11, 2024

Thank you, please remove the indirection/abstraction from the tests (i.e. do not generate the project graph data dynamically). It will be a lot more lines of code (but still not a crazy amount), but a lot simpler to read which is the priority with tests

@JamesHenry
Copy link
Collaborator

@mpsanchis Thank you this LGTM, I pushed a commit which greatly reduces the churn in test file and keeps things inline. I always prefer to keep additions of tests covering new behaviour separate from refactoring of existing tests where possible https://github.com/nrwl/nx/pull/26491/files#diff-dd7e8070cc17859170fab8e1ff1d38ce36fa3e1bf5cc0ec7928b7d7d0a117480

@JamesHenry JamesHenry merged commit 0018842 into nrwl:master Jun 11, 2024
6 checks passed
@JamesHenry
Copy link
Collaborator

Thanks again @mpsanchis!

Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

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

Successfully merging this pull request may close these issues.

Js release-version: topological ordering iterates all edges per node instead of only its own
2 participants