-
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
FED-2138 Fix createContext typing #875
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
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.
Just a typo and some confusing doc comments... otherwise I'm plus one @greglittlefield-wf
test('createContextInit() returns a correctly typed object', () { | ||
final context = createContextInit<int>(0); | ||
expect(context, isA<Context>()); | ||
expect(context, isA<Context<int>>(), reason: 'should have a non-nullable generic type'); |
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.
NICE
@aaronlademann-wf Good catches on the doc comments, addressed in 2bba27d! |
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.
+10
- Passing CI
Motivation
The current typing for
createContext
is incorrect/unsound.But if you don't provide a default value, the value will be
null
, which isn't aTValue
whenTValue
is a non-nullable type. And also, it's possible to providenull
even whenTValue
is non-nullable.This typing problem is essentially the same as useRef, for which we had to create useRefInit (link), and deprecated the arg to useRef. That way, we won't have to break createContext, and people passing the deprecated argument will get a deprecation hint telling them to move to the new one.
Changes
createContext
to always return contexts with nullable type, since we can't guarantee that a default value was providedcreateContext
, which allows creation of contexts with non-nullable types, and required specifying a default valueuseContext
exampleRelease Notes
Review
See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.
Please review:
QA Checklist
Merge Checklist
While we perform many automated checks before auto-merging, some manual checks are needed: