-
Notifications
You must be signed in to change notification settings - Fork 67
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
FEDX-414 Add APIs and update deprecations in preparation for 7.0.0 release #372
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.
Couple questions, but I'm +1 here
- This should not be a breakage, since as of Dart 2.0 inheriting from Function has had no effect | ||
|
||
#### Potential behavior breakages | ||
- Component and Component2 members `props`/`state`/`jsThis` are late, will now throw instead of being null if accessed before initialized (e.g., in a constructor, final class field, or static lifecycle method). |
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.
This is pretty much guaranteed to break Component
subclasses where lifecycle methods reference those fields, correct?
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.
They'll be initialized for most lifecycle methods, except for "static" ones like defaultProps
, getDerivedStateFromProps
, and getDerivedStateFromError
where you shouldn't ever reference props/state.
Besides those "static" lifecycle methods, they'll be initialized by the time we get to lifecycle methods.
The sequence during component mount is:
- The Dart component class is instantiated
- non-
late
class fields are initialized - Default constructor is called
- non-
props
are initializedgetInitialState
orinitialState
is calledstate
is initialized- All non-static lifecycle methods (
componentWillMount
,componentDidMount
,render
, etc.) are called
For example:
class FooComponent extends Component2 {
FooComponent() {
// props would have always been null here, but in 7.0.0 accessing it throws
print(props);
}
render() => 'foo';
}
class FooComponent extends Component2 {
// props would have always been null when this is initialized, but in 7.0.0 accessing it throws
final something = (props ?? {})['something'];
render() => 'foo';
}
class FooComponent extends Component2 {
getDerivedStateFromProps(Map nextProps, Map prevState) {
// props would have always been null here, but in 7.0.0 accessing it throws
print(props);
}
}
Basically, this shouldn't ever happen unless you're doing something wrong. And if you were doing it wrong, you'd probably get a null exception when trying to read off of them. I think I originally had a note to that effect in here, but scrapped it since I couldn't get wording I was happy with. I'll take a stab at re-adding it.
@Deprecated('7.0.0') | ||
/// > Will be removed when `Component` is removed in a future major release. |
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.
is this removal intentional?
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.
Good catch, nope; I'll re-add it
/// > This will be completely removed when the JS side of it is slated for removal (ReactJS 18 / react.dart 7.0.0) | ||
/// > This will be completely removed alongside the Component class. | ||
void componentWillUpdate(Map nextProps, Map nextState) {} |
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.
I'm a little unclear why certain methods are deprecated, and others are not.
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.
I think we deprecated only the ones related to the legacy context, and for the rest deprecating Component
itself seemed sufficient.
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.
+1
612d642
to
a4a30a6
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.
+1 / +10
- Passing CI
@Workiva/release-management-p |
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.
+1 from RM
Motivation
react-dart 7.0.0 will contains some breaking changes to APIs that are not yet deprecated in the 6.x line.
The 7.0.0 WIP branch also relies some API additions that aren't present in 6.x, so we'll want to backport them to ensure consumers can transition to these new APIs before setting their lower bounds to 7.0.0.
Solution
Component
and its associated APIs, which we don't have a timeline for removinghtmlMain
anduseRefInit
QA steps