invertase / melos

🌋 A tool for managing Dart projects with multiple packages. With IntelliJ and Vscode IDE support. Supports automated versioning, changelogs & publishing via Conventional Commits.
https://melos.invertase.dev/~melos-latest
Apache License 2.0
1.15k stars 203 forks source link

`PackageMap.resolvePackages` does not add all the `example` packages in the map. #174

Closed xsahil03x closed 3 years ago

xsahil03x commented 3 years ago

Hey, First of all, thanks for creating melos. We are using it in StreamChatFlutter and it makes our life easier 😄

We recently tried to use the latest pre-release version of melos and we saw some differences in the boostrap command. Previously it used to successfully bootstraps all our packages and their example apps. But now, it bootstraps all our packages but not all our package example apps.

So I looked into the implementation and found that rather than bootstrapping all the packages and examples. It now only bootstraps unique packages.

eg:

packages:
    - stream_chat
    - stream_chat/example
    - stream_chat_flutter
    - stream_chat_flutter/example

We expect that melos will bootstrap all the provided packages. But it only bootstraps 3 packages, overwriting the example because of the same name.

final name = pubSpec.name!;

packageMap[name] = Package._(...);
Salakar commented 3 years ago

👋, thanks for the bug report - I see - perhaps we should change the packageMap to store by package path rather than naming - is this something you're willing to send a PR for?

rrousselGit commented 3 years ago

Oh, that's interesting.

+1 on using the full package path as key.

rrousselGit commented 3 years ago

I was about to fix this, but this raises an interesting question:

Do we really want to support multiple projects with the same name in a melos workspace?
Because I could see this be quite confusing if say another package in the workspace did:

dependencies:
  example:

Ultimately, a workaround to this issue is to have a unique name for every example. Kind of like what FlutterFire does, with cloud_firestore_example vs firebase_auth_example

Salakar commented 3 years ago

I was about to fix this, but this raises an interesting question:

Do we really want to support multiple projects with the same name in a melos workspace? Because I could see this be quite confusing if say another package in the workspace did:

dependencies:
  example:

Ultimately, a workaround to this issue is to have a unique name for every example. Kind of like what FlutterFire does, with cloud_firestore_example vs firebase_auth_example

Now that you've mentioned it I'm not 100% sure on this either, on FlutterFire for example we made sure to have unique package names even for the examples. Adding support for this in Melos as well could also potentially cause ambiguity with various package name based filters in Melos such as --scope or --ignore, e.g. I want to exclude a package named example but there's more than one and --ignore="example" will exclude all of them without any way to be specific since it's name based.

Maybe a better change if we don't support this (which I'm leaning towards now) is to do a key existence check before we do a packageMap[name] = assignment and print a duplicate package name warning (maybe even an error?)

Is there any particular use case to have multiple packages of the same name in a repo that you had in mind @xsahil03x?

rrousselGit commented 3 years ago

I agree on the "throw if there's a name duplicate"

If we later want to change this, we can come back to this later in the future and remove the exception. This wouldn't be breaking.
Meanwhile, folks who make the mistake will have a nice error message.

xsahil03x commented 3 years ago

Hey @Salakar, @rrousselGit We can't use any other name for example because pub.dev uses that path in order to show the package example. https://dart.dev/guides/libraries/writing-package-pages#tip4

Ultimately, a workaround to this issue is to have a unique name for every example. Kind of like what FlutterFire does, with cloud_firestore_example vs firebase_auth_example

And about flutter_fire, I don't think they use any different name than example. https://github.com/FirebaseExtended/flutterfire/tree/master/packages/cloud_firestore/cloud_firestore/example

rrousselGit commented 3 years ago

The important part isn't the folder name, but the package name inside the pubspec.

You can still have them inside an example folder, but give your examples a unique package name:

# my_package/example/pubspec.yaml
name: my_package_example
...

This is what FlutterFire does

https://github.com/FirebaseExtended/flutterfire/blob/cc13c55d3bdd70c53a864d0cbaa0240d81f1d7c0/packages/cloud_firestore/cloud_firestore/example/pubspec.yaml#L1

xsahil03x commented 3 years ago

Thanks @rrousselGit, I think that will solve the problem 👍🏼