Make bootstrap, exec and run commands execute packages in dependency order by default#412
Make bootstrap, exec and run commands execute packages in dependency order by default#412doug-wade merged 8 commits intolerna:masterfrom seansfkelley:toposort
Conversation
|
Permissions errors on the AppVeyor build per usual. 😞 |
|
This probably clashes awkwardly with #386; I would prefer that one get merged first and then I'll update this as appropriate. |
|
Just wanted to say that I'd love #412 to get merged. Very typical in the micro-repos that I create to have a number of small packages and one that bundles them all into a single place. Would love to be able to run my build scripts and then perform a publish. But as you know, |
|
@seansfkelley #386 has merged. Please rebase this when you get a chance. |
Conflicts: src/commands/BootstrapCommand.js src/commands/ExecCommand.js src/commands/RunCommand.js test/PackageUtilities.js
|
@gigabo updated! |
| bootstrapPackages(callback) { | ||
| this.filteredGraph = PackageUtilities.getPackageGraph(this.filteredPackages); | ||
| this.logger.info(`Bootstrapping ${this.filteredPackages.length} packages`); | ||
| this.batchedPackages = this.flags.toposort |
There was a problem hiding this comment.
Open question: bootstrap used to toposort by default. For consistency, I have tentatively made toposort opt-in-only across the board. Is this behavioral change (1) a good idea? and (2) sufficiently breaking for a beta version to justify special-casing bootstrap?
I am tempted to say (1) yes and (2) no, assuming there is a way we can call out to existing beta users that the default behavior has changed (maybe add a log line in bootstrap if the option was not specified?).
There was a problem hiding this comment.
Oh, boy. Yeah, I appreciate that making it opt-in for bootstrap is consistent.
I guess a repo that does need it can just add it to lerna.json.
There was a problem hiding this comment.
So are there any changes I should make here, or is it good to go?
There was a problem hiding this comment.
@seansfkelley I think it is more important to be right than it is to be fast, since it is much slower to get a build failure and have to debug and rebuild toposorted than it is to waste the excess time running toposorted when you don't strictly need it, but that we should strive to "do the right thing" as often as possible. I would actually propose two flags, --sorted and --unsorted (I don't think the topo prefix helps much) that overrides the default behavior, and then set a reasonable default per-command (that is to say, run run, exec and bootstrap sorted and the rest unsorted). By making any command where sorting might be required sorted, we save less experienced users from erroring/rebuilding as often as possible; by providing flags to override the default behavior of any command, we allow more savvy users to use lerna in ways we haven't anticipated yet to do clever new things.
There was a problem hiding this comment.
The most foolproof correct way to implement this flag would then be to default it on always, and provide a --no-sort option to disable it, no? (Agree that "topo" is probably noisy; I think "no sort" is phrased the most CLI-like though.)
There was a problem hiding this comment.
@seansfkelley That sounds great.
Thanks for the suggestion @doug-wade.
|
Is there anything that's blocking this PR? It's becoming important for our team as we're adding more and more packages to monorepo, and doing fresh bootstrap isn't option anymore. The problem is when you have packages A and B (depends on A), and you introduce new package C being used by first two, you have to manually toposort everything in your head and do scoped bootstrap to save time. |
|
@xaka I think @seansfkelley was planning to make some tweaks before we merge. |
|
@gigabo it should be good to go now. |
|
Awesome! Thanks @seansfkelley! |
gigabo
changed the title
|
@seansfkelley I updated the title to try to reflect the current state of what this provides. Please improve it if you've got better wording. It will go into the changelog entry for this PR. |
seansfkelley
changed the title
seansfkelley
changed the title
|
Okay, changed (a bit wordier but more precise). |
|
Awesome work! Looking forward to the merge. |
|
Would be nice to have another pre-release version with this change in 🎆 It's big and important one. |
doug-wade
left a comment
doug-wade
left a comment
There was a problem hiding this comment.
Hi @seansfkelley I'm super excited to get this in. Would you mind rebasing it (hopefully one last time) and we'll get it merged?
|
@doug-wade done! |
|
This thread has been automatically locked because there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
5 participants
No description provided.