-
Notifications
You must be signed in to change notification settings - Fork 58
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
[OLD] FED 2299 Utilize new ReactNode typedef #893
Changes from 9 commits
7f1e7d6
86bfdaf
5e84679
e752442
f0f3666
daed633
5b00b97
374ab79
719394d
5ce5849
d9129a3
c5119ff
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,11 +67,11 @@ mixin SuspensePropsMixin on UiProps { | |
/// The actual UI you intend to render. If children suspends while rendering, the Suspense boundary will | ||
/// switch to rendering fallback. | ||
@override | ||
/*ReactNode*/ List<dynamic>? get children; | ||
List<ReactNode>? get children; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to above, the changes to props in this component are technically breaking changes, but I think we could get away with these in particular since, by comparison, there should be almost no components that read the children/fallback props from this props mixin. |
||
|
||
/// An alternate UI to render in place of the actual UI if it has not finished loading. Any valid React node is | ||
/// accepted, though in practice, a fallback is a lightweight placeholder view, such as a loading spinner or skeleton. | ||
/// Suspense will automatically switch to fallback when children suspends, and back to children when the data is ready. | ||
/// If fallback suspends while rendering, it will activate the closest parent Suspense boundary. | ||
/*ReactNode*/ dynamic fallback; | ||
ReactNode fallback; | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Unfortunately this is a breaking change, since consumers will no longer be able to dynamically invoke methods on children (which they probably shouldn't be doing, and instead using proper type checks).
And I think it might mess up implicit casting in some cases too, not 100% sure.
I'd just be surprised if there aren't some usages out there that would be broken.
So, I'd say either we revert this change, or if we really wanted to, perhaps we could do some consumer static analysis testing to see if it affects anything.
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.
Kicked off a static analysis check
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 check did not do well. I'll revert this.
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.
c5119ff