CPLAT-2272: Add UiComponent2 (JS-backed maps)#271
Merged
rmconsole6-wk merged 43 commits into3.0.0-wipfrom Mar 20, 2019
Merged
Conversation
# Conflicts: # lib/over_react.dart # lib/src/component_declaration/component_base.dart # lib/src/transformer/impl_generation.dart # pubspec.yaml
…previous member order
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
aaronlademann-wf
requested changes
Mar 15, 2019
Contributor
aaronlademann-wf
left a comment
There was a problem hiding this comment.
Should probably go ahead and deprecate all the old component classes.
| ..writeln(' // of `_$propsOrState` in the constructor body is necessary to work around a DDC bug: https://github.com/dart-lang/sdk/issues/36217') | ||
| // TODO need to remove this workaround once https://github.com/dart-lang/sdk/issues/36217 is fixed get nice dart2js output | ||
| ..writeln(' $plainMapImplName(Map backingMap) : this._$propsOrState = {}, super._() {') | ||
| ..writeln(' this._$propsOrState = backingMap ?? {};') |
Contributor
There was a problem hiding this comment.
Any benefit to making the fallback be const {}?
Contributor
Author
There was a problem hiding this comment.
We can't because this map backs the returned props object, which can be mutated.
CPLAT-3300: CP Code Review and PR Merging Team agreement update
aaronlademann-wf
approved these changes
Mar 20, 2019
| SyntheticWheelEvent; | ||
|
|
||
| // FIXME use public entrypoint | ||
| // FIXME 3.0.0-wip use public entrypoint |
Contributor
There was a problem hiding this comment.
|
|
||
| export 'package:over_react/src/component_declaration/component_type_checking.dart' show isComponentOfType, isValidElementOfType; | ||
|
|
||
| part 'component_base/component_base_2.dart'; |
aaronlademann-wf
approved these changes
Mar 20, 2019
Contributor
aaronlademann-wf
left a comment
There was a problem hiding this comment.
QA +1
@Workiva/release-management-pp
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ultimate problem:
Add over_react analogues of
Component2, introduced in react-dart to implement JS-object-based props/state and to serve as the basis for React 16 lifecycle methods.See Workiva/react-dart#161 for more context
Changes
UiComponent2/UiStatefulComponent2` base classesGenerate an additional UiProps class implementation for each component that can only be backed by JS maps, and override the return type of
typedPropsFactoryJs/propsto match this.This allows dart2js to make some optimizations. For instance:
For the access of
props.isOpenwithin a component, as we start to inline things (for the purposes of understanding what's going on under the hood), then it looks like this:So, we're eventually accessing a property on the
Map backingMap.But if
backingMapis aJsBackedMap, we can go one layer deeper:Now, onto dart2js, which performs actual inlining in certain cases (like prop accesses).
When dart2js only knows that
backingMapis aMap, it emits the following code, wherein$index$asxperforms type-checking on the map and key, and then performs an interceptor lookup on the backing map before finally calling into the actual key lookup function.However, when dart2js knows that
backingMapis aJsMap, it can skip that step and emitcode that directly accesses the JS property
UiComponent2to serve as a smoke branch as we finalize this work.@Requiredannotation that was being tested, as well as related testsweb/to useUi(Stateful)Component2web/withUiComponent2versionsTesting
web/component2/examples and verify their props/state show up in the dev tools as expected!Potential areas of regression: