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(core): fix handling of simple dependsOn target string #16932

Merged
merged 1 commit into from
May 11, 2023

Conversation

acburdine
Copy link
Contributor

Fixes #16931

  • if a project has a dependsOn string with only a target that has the same name as another project, it is now treated correctly as a reference to a target in the same project

This was the simplest approach I could think of to fix the issue and ensure the functionality remains the same as is currently documented.

@vercel
Copy link

vercel bot commented May 11, 2023

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

Name Status Preview Comments Updated (UTC)
nx-dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 11, 2023 11:47am

Copy link
Member

@AgentEnder AgentEnder left a comment

Choose a reason for hiding this comment

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

This makes sense to me, thanks for the contribution 🎉

Can you add a unit test and address the change I requested?

Comment on lines +75 to +77
if (!segments.length) {
return { target: maybeProject };
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (!segments.length) {
return { target: maybeProject };
}
if (segments.length == 1) {
return { target: maybeProject };
}

This condition as written would never be hit, since there is always at least 1 segment (that just happens to be the target name most of the time).

It's OK IMHO to assume it's a target name when only one segment is present, so updating the condition like above should suffice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AgentEnder I think this code is correct as written - a couple lines above there's the [maybeProject, ...segments] = splitByColons(targetString) line. Unless I'm misunderstanding how the split function works, it seems that segments does have the possibility to be empty 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also for what it's worth - I tested the change as I originally wrote it in my monorepo (via patch-package) and it did fix the issue I was experiencing.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes. That's what I get for checking on my phone!

Thanks for the contribution!

closes nrwl#16931
- if a project has a `dependsOn` string with only a target that has the same
  name as another project, it is now treated correctly as a reference to
  a target in the same project
@acburdine acburdine changed the title fix(task-graph): fix handling of simple dependsOn target string fix(core): fix handling of simple dependsOn target string May 11, 2023
@AgentEnder AgentEnder merged commit d853ba2 into nrwl:master May 11, 2023
@github-actions
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 May 17, 2023
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.

dependsOn shorthand broken when target name matches name of a project in the same repo.
2 participants