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(run): fully defer to Nx for dep detection when nx.json exists #3345

Merged
merged 7 commits into from Sep 28, 2022

Conversation

fahslaj
Copy link
Contributor

@fahslaj fahslaj commented Sep 27, 2022

Description

  • Only sends "targetDependencies" from Lerna to Nx when nx.json does not exist.
  • Updates the warning when using --include-dependencies to indicate that Lerna will send Nx additional dependencies to run.
  • Updates docs to be more clear on the differing behavior.
  • Adds a note to the docs when installing Nx that lerna run will behave differently and links to the page describing the differences.

How Has This Been Tested?

This change has been covered by e2e tests and tested manually.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (change that has absolutely no effect on users)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@fahslaj fahslaj marked this pull request as ready for review September 27, 2022 20:10
@vsavkin vsavkin merged commit fef2ae6 into lerna:main Sep 28, 2022
@fahslaj fahslaj deleted the only-send-target-deps-if-no-nxjson branch September 28, 2022 13:45
@alasdairhurst
Copy link

alasdairhurst commented Sep 29, 2022

FYI this introduces a regression in 5.5.4 where i have an nx.json (default file) and suddenly the order of lerna run (lerna run build) is back to being alphabetical rather than topological order.

@JamesHenry
Copy link
Member

@alasdairhurst please open an issue with a reproduction

@JamesHenry
Copy link
Member

Actually @alasdairhurst if you could provide the contents of your nx.json initially?

@alasdairhurst
Copy link

alasdairhurst commented Sep 29, 2022

{
	"tasksRunnerOptions": {
		"default": {
			"runner": "nx/tasks-runners/default",
			"options": {
				"cacheableOperations": []
			}
		}
	}
}

@alasdairhurst
Copy link

@alasdairhurst please open an issue with a reproduction

We have quite a complex setup (There's 46 modules with a number of interdependencies.) but i'll see if i can find time to set up something and try to build a reproduction to help you out if you're not sure about the root of the issue yet. Since nobody else has reported the issue surprisingly, it seems like it could be slightly more in depth than i was expecting.

Let me know if there's any other info/logs/config that could help you pinpoint the issue that i can provide from my current monorepo.

@JamesHenry
Copy link
Member

Ah right, no it's ok @alasdairhurst you were right in your initial point. This change here has made it so that the presence of nx.json is considered a more significant "buy in" to wanting the benefits of Nx's much smarter task runner.

Nx's task runner doesn't just understand dependencies at a package/project level, it also understands them at a script (a.k.a "target") level.

Therefore when the nx.json is present we now expect folks to configure the target relationships (i.e. declare which targets need to be run in order). This is achieved via the targetDefaults property in nx.json - which your config doesn't yet have.

targetDefaults is covered in this section of the docs: https://lerna.js.org/docs/getting-started#target-dependencies-aka-task-pipelines.

For example, if you have a build script in all your packages, here is how you would tell the task runner that build scripts need to be run in topological order:

nx.json

{
  ...
  "targetDefaults": {
    "build": {
      "dependsOn": [
        "^build"
      ]
    }
  }
}

(^ means "from dependencies")

We are adding a helper command to the lerna CLI which will give you an interactive prompt to help you generate this configuration (and will then update the docs accordingly once that lands), but until then you'll just need to configure your nx.json based on the above.

Please let me know if you face any issues with that and I can help out!

@alasdairhurst
Copy link

alasdairhurst commented Sep 29, 2022

Thanks a lot for the detailed description! I'll look into applying this as you suggested.

I'll explain how i recall that got to this configuration in case the current setup i have was a bit of an oversight when making this change. Maybe there could be some improvements to make in the default nx configuration?

I updated to the latest version of lerna when nx got integrated. At this point, i was able to install nx, set "useNx" to true in lerna.json and everything worked as per the migration guide (only followed the required steps), and get the cool new behavior for lerna run out of the box. At this point nx.json was not required.

A few weeks ago, the change in behavior i'm seeing currently suddenly appeared (build order changed and everything started running in parallel without taking into account dependencies). running "nx init" (had to run it in a different project since it was already installed in my monorepo) allowed us to generate the default nx.json as i posted above, and this returned to the previous lerna run behaviour. I believe it was this (just found it today): #3336

Then it seems like adding the nx.json as a solution was a double-edged sword since the behavior reverted again in the last release and impacts when you have the (default) nx.json :)

Note that the nx.json created from nx init, and the nx.json in the lerna migration guide (https://nx.dev/recipe/lerna-and-nx) doesn't include targetDefaults, but at the time that I upgraded, this was the default behavior, now it won't be for users following the same process, and i presume by default if they follow the optional step to create nx.json and leave it as-is then nx run probably won't work as expected.

I'll leave the rest up to you guys. Just letting you know my experience and hopefully it is useful.

@JamesHenry
Copy link
Member

@alasdairhurst thank you for the feedback, we do agree and want to help bridge the gap, so Austin is working on a refinement to it here: #3349

The new add-caching command is also in draft here: #3350

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.

None yet

4 participants