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

feat: Sync common dependency versions between packages #526

Merged
merged 18 commits into from
Oct 22, 2023

Conversation

lohnn
Copy link
Contributor

@lohnn lohnn commented May 17, 2023

NOTE: I'm adding this as a draft PR as docs and tests are not yet added and I haven't yet added feedback if the user turned on the flag but didn't provide the common_packages.yaml-file. But I wish for feedback on my solution anyways.

Description

Added support for setting versions for common/shared dependencies that will be applied to all packages in a Melos monorepo project. Implements #94.

A new bootstrap config flag (shareDependencies) is added to start using this new feature, and then the dependency versions for dependencies and dev_dependencies in common_packages.yaml will be applied to all packages that contains said dependency.

Type of Change

  • feat -- New feature (non-breaking change which adds functionality)
  • 🛠️ fix -- Bug fix (non-breaking change which fixes an issue)
  • ! -- Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 refactor -- Code refactor
  • ci -- Build configuration change
  • 📝 docs -- Documentation
  • 🗑️ chore -- Chore

@docs-page
Copy link

docs-page bot commented May 17, 2023

To view this pull requests documentation preview, visit the following URL:

melos.invertase.dev/~526

Documentation is deployed and generated using docs.page.

@lohnn
Copy link
Contributor Author

lohnn commented May 17, 2023

Another question: should Melos maybe sync the Dart and Flutter version in environment as well?

@vkammerer
Copy link

My two cents:

  • Instead of a dedicated separate file, we could include these properties in melos.yaml. It is a feature of melos, so there is no reason to exclude it from the melos settings. At a later point, this could potentially also be a part of pubspec.yaml if request: add melos: section to root pubspec.yaml instead of requiring melos.yaml in Melos v3 #472 is implemented.
  • the name shareDependencies makes sense if the dependencies versions are taken from the root package's own dependencies, as suggested by @blaugold in Request: common dependencies #94 (comment), but since your implementation follows another approach, there is no "sharing" anymore. Instead, a more appropriate name could be assignCommonDependencies or similar.
  • you could avoid calling https://github.com/invertase/melos/pull/526/files#diff-b5438310509b0fb6f04ef9776c2b9d3ec2a18fe0e0bc7165cdda954148ad9a99R240-R243 if dependenciesUpdated == 0 && devDependenciesUpdated == 0, (which will be the case most of the time, since melos bootstrap is likely to be run more often than common dependencies version are updated).
  • There is a mixed usage of the terms "package" and "dependency", which is confusing. I may be wrong, but it looks to me that a "package" refers to a local package managed by melos, while a "dependency" refers to an entry of the "dependencies" and "dev_dependencies" fields. If that is indeed the case, there are several usages of "package" that should be replaced with "dependency":
    • if we wanted to go with a separate file, I guess it should be named "common_dependencies.yaml" rather than "common_packages.yaml"
    • _updatePackages -> _updateDependencies
    • packagesToUpgrade -> dependenciesToUpgrade
    • etc
  • the naming of dependenciesUpdated and devDependenciesUpdated sound like booleans to me. I would suggest updatedDependenciesCount and updatedDevDependenciesCount to be more explicit.

But all these are details, compared to the more important design decision of whether we want to share the root package's dependencies or use separate fiels as you have done. As I have already expressed in #94 (comment) , I am more in favour of new fields, but it would be nice to hear @blaugold 's opinion on this design choice.

@lohnn
Copy link
Contributor Author

lohnn commented Jun 2, 2023

I would also love to hear what @blaugold has to say as I should have some time over to continue with this PR next week.

@blaugold
Copy link
Collaborator

blaugold commented Jun 6, 2023

Sorry for not responding earlier! 😔 The past few weeks have been really busy.

@lohnn Thanks for working on this!

Another question: should Melos maybe sync the Dart and Flutter version in environment as well?

Great idea. I suspect it's just a question of time until someone asks for this, especially with support for common dependencies.

@vkammerer Thanks for the detailed input. You convinced me that putting the common dependencies into the root pubspec is probably not a good idea. I feel like the next best option is to put them into melos.yaml. Like you, I don't really see a reason not to. We already maintain a JSON schema for melos.yaml, which can be used for autocompletion and validation. Extending that is much easier than introducing a new type of file.

Regarding naming, I would be fine with mirroring that of pubspec.yaml under the bootstrap command options:

command:
  bootstrap:
    environment:
      ...
    dependencies:
      ...
    devDependencies:
      ...

@lohnn
Copy link
Contributor Author

lohnn commented Jun 13, 2023

Heh, it's all good! I'm getting married soon and know exactly what you mean. Thankfully I can do some of this job during work hours (wish I could say the same for my own package, but we currently don't use it at work 😅)

Great idea. I suspect it's just a question of time until someone asks for this, especially with support for common dependencies.

Then I'll add it to the TODO list.

Regarding putting the common dependencies in melos.yaml file I only have one gripe. The melos.yaml file is, at least in our project, already quite bloated clocking in at just short of 200 ilnes. Adding all the common dependencies would increase the size of this file even more.

On the flip side, we all use IDEs and can collapse parts of the file if we want to make it more readable, so if you still wish for the common dependencies (and version number) to live in the melos.yaml file, I'll update this PR accordingly :)

Regarding naming, I would be fine with mirroring that of pubspec.yaml under the bootstrap command options:

I imagine you mean it should be:

command:
  bootstrap:
    environment:
      ...
    dependencies: 
      ...
    dev_dependencies: # instead of devDependencies
      ...

@vkammerer
Copy link

@lohnn I would much prefer the simplicity of a few lines added to melos.yaml (which would be visible only when you view that file) than having to deal with yet another config file just for this purpose (which would always be visible when you browse your project in your code editor, which is to say almost all the time).

Moreover, the argument about it belonging to melos responsibilities and therefore belonging to melos.yaml still stands.

@lohnn
Copy link
Contributor Author

lohnn commented Jun 13, 2023

Check! I'll take it and update the code accordingly 👍

@lohnn
Copy link
Contributor Author

lohnn commented Jul 18, 2023

I believe the implementation of this is now implemented and fully functional. When updating dependencies or environment it will currently print this to show that pubspec has been updated:

  base_project
  └> Updated environment
     Updated 1 dependencies
     Updated 1 dev_dependencies
  screenshot_tests
  └> Updated environment

Untouched packages will not appear at all to not clutter logging unless a change is actually done.

@blaugold

We already maintain a JSON schema for melos.yaml, which can be used for autocompletion and validation. Extending that is much easier than introducing a new type of file.

Where can I find this to update accordingly?

@blaugold
Copy link
Collaborator

Where can I find this to update accordingly?

The schema is currently maintained here in the repo for the Melos VS Code extension. In the long term, it should be moved here, though (#425).

@blaugold blaugold self-requested a review July 18, 2023 12:56
@lohnn
Copy link
Contributor Author

lohnn commented Jul 19, 2023

And regarding tests for this work. I should look into that I guess?

@lohnn lohnn marked this pull request as ready for review July 19, 2023 07:07
@lohnn
Copy link
Contributor Author

lohnn commented Aug 24, 2023

I have now added a test to validate that the bootstrap dependency sync runs as intended, and the PR is now ready to be reviewed.

@lohnn
Copy link
Contributor Author

lohnn commented Oct 13, 2023

Ping: I would love for this PR to be reviewed.

@blaugold
Copy link
Collaborator

@lohnn Sorry for the delay! I'll review it tomorrow.

Copy link
Collaborator

@blaugold blaugold left a comment

Choose a reason for hiding this comment

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

Looking good mostly!

I've fixed the analyzer issues on main, so please rebase or merge to get the checks passing.

packages/melos/lib/src/commands/bootstrap.dart Outdated Show resolved Hide resolved
packages/melos/lib/src/commands/bootstrap.dart Outdated Show resolved Hide resolved
packages/melos/lib/src/commands/bootstrap.dart Outdated Show resolved Hide resolved
packages/melos/lib/src/commands/bootstrap.dart Outdated Show resolved Hide resolved
packages/melos/lib/src/commands/bootstrap.dart Outdated Show resolved Hide resolved
packages/melos/lib/src/common/utils.dart Outdated Show resolved Hide resolved
docs/commands/bootstrap.mdx Outdated Show resolved Hide resolved
@lohnn
Copy link
Contributor Author

lohnn commented Oct 18, 2023

Thank you for your review and all your comments should now be taken care of :)

Copy link
Collaborator

@blaugold blaugold left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks! ❤️

@blaugold blaugold merged commit 39e5e49 into invertase:main Oct 22, 2023
10 checks passed
@lohnn lohnn deleted the feat/common-dependenices branch December 4, 2023 09:49
@feinstein
Copy link

feinstein commented Dec 4, 2023

I am trying to find this mechanism in the docs website, but there I can only see a dependencies section in bootstrap. Has the common_packages.yaml file being removed?

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.

4 participants