Updates for Dart 2.0 core library changes (wave 2.2) #45
Conversation
CHANGELOG.md
Outdated
| Changed classes that implement `StreamTransformer` to extend | ||
| `StreamTransformerBase`, and changed signatures of `firstWhere`, `lastWhere`, | ||
| and `singleWhere` on classes extending `Stream`. See also | ||
| https://github.com/dart-lang/sdk/issues/31847 |
There was a problem hiding this comment.
Nit: I'd write this
* ... See also [issue 31847][sdk#31847].
[sdk#31847]: https://github.com/dart-lang/sdk/issues/31847
lib/src/typed/stream.dart
Outdated
| Future<T> firstWhere(bool test(T element), | ||
| {Object defaultValue(), T orElse()}) => | ||
| _stream.firstWhere(_validateType(test), | ||
| defaultValue: defaultValue ?? orElse); |
There was a problem hiding this comment.
I think this should be defaultValue: defaultValue, orElse: orElse. I don't want this class to have any logic beyond what's necessary to get the types to work out.
There was a problem hiding this comment.
I want to (need to?) land this before @lrhn lands the breaking change in the SDK, so currently there is no orElse on stream to pass this to. I will file an issue to follow up with a PR removing the defaultsTo argument and this logic here after the SDK changes land.
There was a problem hiding this comment.
My understanding from talking with @srawlins at Dart Conf was that we were just going to have this package briefly be in a broken state on master.
There was a problem hiding this comment.
Ah, I see. Yes, I'm fine with that solution if you prefer. It does reduce the number of PRs.
There was a problem hiding this comment.
@keertip @srawlins If I make all of the changes in one go here and leave it on a branch, does that work for testing internal code? I think that means you would have to test with an atomic CL, since you couldn't roll this package before you roll the SDK. You would then presumably also have to roll this package with the SDK when we finally land. Does that work, or should we do these changes in two passes?
There was a problem hiding this comment.
Yeah that works with testing internal code. It... should work for SDK as well, right? orElse can land, and aync's version can bump in DEPS, all in the same changelist.
There was a problem hiding this comment.
Thinking about it, I guess I could push multiple commits to this branch, and then we could roll internal to the first tagged commit (this version), then roll internal to the second tagged commit (the final version). As far as this package is concerned, there is just one PR with a bunch of commits, but internally we can point to different hashes in this branch at different points in the process.
There was a problem hiding this comment.
Do we need multiple commits? Can't both the SDK and internal atomically add orElse and bump their dependency on async in the same commit?
There was a problem hiding this comment.
We want internal to be green before we land the changes in the SDK.
To get internal green, we need to run presubmits, fix, iterate.
It makes running presubmits easier if we can roll a dev.20 compatible version internally.
So I think having two commits to reference here for now would be better. Since we're referencing them off the branch, I don't think it should matter in the long run - we'll just leave this branch sitting here for now, and land the PR atomically as usual once everything is ready.
|
2.0.0-dev.23.0 is now released on windows so any ddc builds depending on this package are now broken at the latest dev. I will try pushing a commit with desired changes and we can get it merged/published? |
|
Your changes LGTM. |
|
I've verified locally that tests and analysis pass with dart-lang/sdk@365f7b5. |
This makes the initial changes necessary to make this package compatible with the corelib wave 2.2. changes. See dart-lang/sdk#31847 for more details.