-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Make bootstrap, exec and run commands execute packages in dependency order by default #412
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! |
this.logger.info(`Bootstrapping ${this.filteredPackages.length} packages`); | ||
this.batchedPackages = this.flags.toposort |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So are there any changes I should make here, or is it good to go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping @gigabo: thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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! |
@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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
No description provided.