-
Notifications
You must be signed in to change notification settings - Fork 708
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
[3.2] Update breaking changes page #5331
Conversation
Visit the preview URL for this PR (updated for commit 96b1a17): https://dart-dev--pr5331-3-2-breaking-changes-9xn7dje2.web.app (expires Tue, 21 Nov 2023 23:33:32 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: d851bc446d3c4d7394c5406c6f07255afc7075f3 |
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.
Thanks for working on this @MaryaBelanger, looks great!
I think a few additional adjustments might be nice:
- It depends a bit on what you want to call a breaking change, but private final field promotion can result in the
unnecessary_non_null_assertion
diagnostic (and others) triggering on code that previously passed analysis, but with the new language version now has a warning due to the unnecessary use of!
or similar. This can technically result in CI failures. @bwilkerson might have more well-defined thoughts on if scenarios like these are worth calling out as breaking changes. - I think DDC and dart2js entries should referenced as
Development JavaScript compiler (DDC)
andProduction JavaScript compiler (dart2js)
respectively since the underlying compiler names are less and less visible to users. - While
waitFor
has been deprecated for many releases, before 3.2 it just said "This functionality is incomplete and may be removed in a later version". Now in 3.2 that decision has been made and it explicitly states it "is deprecated and will be removed in Dart 3.4". I think it would be good to call this deprecation out again so people are aware ([breaking change] Remove dart:cli waitFor experiment sdk#52121).
Co-authored-by: Parker Lougheed <parlough@gmail.com>
Interesting, also curious to hear if @bwilkerson or maybe @stereotype441 thinks it'd be useful and/or accurate to include that info here.
Do you think I should update the whole page or just this release? If their underlying names were better known at some point maybe it makes sense to leave old entries since this is somewhat of a chronological reference.
Thank you! Nice catch |
If the goal of this page is to inform developers of changes they may have to account or prepare for, I would personally say it's worth a mention, even if newly triggered diagnostics are less breaking than other changes. Happy to hear other's thoughts though :)
I think it's fine to keep the change to this release and going forward, but the repositioning as more of an implementation detail has been going on for a few releases. |
I agree with that sentiment; this page is for users who'd expect to be alerted of something like that. If someone thinks it shouldn't be mentioned, I can take it out but until then it's in :) @parlough Would you put it under |
Looking at the top of the file, we do say "This page includes the following types of breaking changes ... Language versioned". So I definitely agree with @parlough that it's worth calling out the change. As to what we should say about it, I'm not sure. Technically, any change to type analysis can potentially cause any number of errors, warnings, or lints to occur, because a change in a type in one place can cause the meaning of the code that follows it to change. For example, when we turned the feature on in our internal code base, there was one compile error (out of millions of lines of Dart code), because the type of a local variable changed. Here's a contrived example of how that might occur: class C {
final int? _x;
C(this._x);
f() {
if (_x != null) {
var y = _x; // Used to have type `int?`, now has type `int`
...
y = null; // Used to be ok, now is an error
}
}
} But it's so rare for something like this to crop up that I'm not sure whether it's worth mentioning; I worry that it would be a lot of wasted mental effort for people to understand this example, and why it's broken, when there's such a tiny chance of their own code having similar problems. In practice, we did see a fair number of these three kinds of warnings:
Here's an example that illustrates all three: class C {
final num? _x = null;
void test() {
if (_x != null) {
print(_x! * 2); // unnecessary_non_null_assertion
print(_x?.abs()); // invalid_null_aware_operator
}
if (_x is int) {
print((_x as int).bitLength); // unnecessary_cast
}
}
} |
Good question. My first thought is There is the one compilation error that @stereotype441 would make sense under Language, but since it is so rare, I feel it's ok to skip that one. |
I love the example that Paul used to illustrate all three potential warnings that may start being triggered. Can we include that @MaryaBelanger or does it not fit this page? |
Added it! This page is supposed to eventually be like Flutter's breaking changes page, where they link out to longer form explanations and migrations, so that kind of example is actually exactly with the purpose of this content (and since we don't utilize those kinds of extra-page-explanations yet, sticking it right on the page is the next best thing :)) |
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.
Thanks for this @MaryaBelanger and those adjustments :D
Feel free to land this one whenever, it doesn't need to wait for the 3.2 release.
- Moved "not yet released to stable" contents to "released in 3.2" section, and added newer entries from the changelog. - Added 3.3.0 changelog contents to "not yet released in stable" --------- Co-authored-by: Parker Lougheed <parlough@gmail.com>
Do not merge until 3.2
fyi @leafpetersen