search

dart-archive / stagehand

Dart project generator - web apps, console apps, servers, and more.
https://pub.dev/packages/stagehand
BSD 3-Clause "New" or "Revised" License
648 stars 118 forks source link

recommend the `any` constraint for package:test #657

Closed devoncarew closed 4 years ago

devoncarew commented 4 years ago

This aligns with the new guidance from the dartdev create command. It also means that our version recommendation won't get out of date.

devoncarew commented 4 years ago

https://dart-review.googlesource.com/c/sdk/+/139680

kevmoo commented 4 years ago

I hate having "any" anywhere.

If we do breaking changes to pkg:test, folks will break randomly and not understand why!

On Mon, Mar 16, 2020 at 9:09 AM Devon Carew notifications@github.com wrote:

https://dart-review.googlesource.com/c/sdk/+/139680

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/dart-lang/stagehand/pull/657#issuecomment-599622559, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAEFCSF7KTFF2BBJI5KZL3RHZFMDANCNFSM4LMMPWVQ .

mit-mit commented 4 years ago

If we do breaking changes to pkg:test,

If we do breaking changes, we should do them sparingly, and pre-announce them. Just like any other breaking change.

kevmoo commented 4 years ago

If we do breaking changes to pkg:test,

If we do breaking changes, we should do them sparingly, and pre-announce them. Just like any other breaking change.

Absolutely.

But we should also not have folks fall into breaking changes by running pub upgrade. That's what you get when you have an any constraint

devoncarew commented 4 years ago

cc @jakemac53 and @grouma, who were in the discussion for what to recommend to people for the dartdev test command.

I think we should unify what stagehand does and what dartdev test does. Here's the current impl. of dartdev test: https://github.com/dart-lang/sdk/blob/master/pkg/dartdev/lib/src/commands/test.dart#L53 - it's been moved to using any. Stagehand uses ^1.6.0: https://github.com/dart-lang/stagehand/blob/master/templates/console-full/pubspec.yaml#L14.

I think I'm gravitating towards what Kevin recommends - that we don't use any, which would technically break the user's project if we ever released a package:test 2.0.0.

In order to not have to update the code w/ package:test releases - to reduce our maintenance burden and the chance for things to get out of date - I think we should change to recommend the last major version constraint - ^1.0.0. This won't need updating frequently, and will resolve to whatever version solves with their other packages.

jakemac53 commented 4 years ago

This package can much more safely recommend a real version other than any, because it isn't a part of the SDK. We can update it at any time and people can always re-activate this package to get the newer constraint.

An SDK lives forever so we cannot imo suggest anything other than any there. That is the only reason I suggest any for that case.

tldr; I actually think the constraints are sufficiently different on these two use cases that its fine to diverge.

grouma commented 4 years ago

+1 to what @jakemac53 said. The likelihood that we break individuals that use package:test as a dev dependency is very low. Breaking changes are more for those that extend the test runner, e.g. Flutter / Google3.

devoncarew commented 4 years ago

because it isn't a part of the SDK. We can update it at any time and people can always re-activate this package to get the newer constraint.

Note that package:stagehand is effectively part of the SDK. It's what dartdev create uses, pinned to a specific version of stagehand. I think we should align what stagehand and dartdev test recommend.

An SDK lives forever so we cannot imo suggest anything other than any there.

I don't understand that. We can vend a specific version of stagehand in (to the SDK repo), that recommends a specific version range of test (^1.0.0), and we can know that that version solves w/ the current SDK version that we're building.

jakemac53 commented 4 years ago

Note that package:stagehand is effectively part of the SDK. It's what dartdev create uses, pinned to a specific version of stagehand. I think we should align what stagehand and dartdev test recommend.

Oh - in that case then I retract my previous comment, we should use any.

I don't understand that. We can vend a specific version of stagehand in (to the SDK repo), that recommends a specific version range of test (^1.0.0), and we can know that that version solves w/ the current SDK version that we're building.

It will give you some compatible version but it could be arbitrarily old. The newer versions might very well also be compatible.

Adding a constraint on an old version is very likely to limit what versions of other packages you can get (due to that old version having old version constraints on its dependencies). It could even cause you to not be able to get a version solve at all.

devoncarew commented 4 years ago

Closing as we don't want to recommend any for general development.

We may do it for very targeted use cases, where we can't predict which package version should solve w/ the sdk version and the user's other packages.