-
Notifications
You must be signed in to change notification settings - Fork 51
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
Migrate to null safety and support Dart 3 #105
Conversation
52afd1b
to
bbc7194
Compare
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.
LGTM! Thanks! Left some comments but mostly as notes. I'm going to try to merge this now.
_hashCode = url.hashCode; | ||
} | ||
|
||
factory Destination.fromMap(Map<String, Object> map) { |
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.
The reason I had this fromMap
<-> toMap
arrangement was twofold:
- Caution. I thought maybe there are environments that can't move non-primitive objects from isolate to isolate. I guess that's not a problem on Win/Linux/Mac desktop, so that's okay.
- Possible problems with mutability. I remember some headaches with having fatal errors when modifying an object after (or during the time?) it was copied to another isolate. I think it was not the case here.
I like the simplicity, though. Let's try this change, and if we see the problems I list above, hopefully one of us will remember where that might be coming from.
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.
Makes sense, thanks for the context. With the introduction of lightweight isolates(dart-lang/sdk#36097), they also expanded and more explicitly outlined what can be sent(https://api.dart.dev/stable/2.18.5/dart-isolate/SendPort/send.html) so I think platform support should be fine now.
I believe the sent objects are also immediately copied, so I don't think there should be any issues during/after the copy, but I'll be on the look out. There could be further benefits to making more of the messages sent deeply immutable, even if just in terms of following the code and avoiding any of those concerns.
Thanks for the review! I hope to test the changes further and likely make some other cleanup changes over the next few days, but it's nice to see the primary migration be complete. |
@parlough No, thank you! Fantastic job. Please let me know if/when you want to tag a commit with Also, I'd love to add you to collaborators if you don't mind. |
Sounds good. I'll open a PR updating the version to 3.0.0 once I finish my testing. As for adding me as a collaborator, sure! |
To support running and building the site with Dart 3, all of our dependencies need to support strong null safety, with linkcheck being the last one remaining. So I went ahead and updated linkcheck to support null safety in filiph/linkcheck#105 and after some local testing on `dart-lang/site-www` and `flutter/website`, the version seems to be ready to switch to.
All of our dependencies are updated to support Dart 3-alpha, with the most recent being linkcheck in filiph/linkcheck#105 and #4502. We can re-enable beta as well once a 3.0 build is released to beta. Fixes #4424
Linkcheck needs to migrate to support sound null safety to allow developers with a Dart 3 dev SDK to run it.
This PR also moves away from a from-to-map based solution for passing messages between isolates. It was a bit messy once migrated to null safety, and the objects can be passed directly instead. To maintain the same system where workers stay alive due to their HTTP connections, messages are now specified within a
WorkerTask
with aWorkerVerb
enum specifying its type. Messages are shared when immutable and automatically copied when not. I didn't run any benchmarks, but I imagine it may result in some speed ups too due to avoiding some unnecessary object constructions as well as simpler comparisons.\cc @domesticmouse and @khanhnwin As this will be needed for dart-lang/site-www (sooner) and flutter/website (later)