Skip to content
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-1886 Null-safe props: runtime validation of required props #862

Merged
merged 28 commits into from
Dec 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
e5be48d
Add initial validateRequiredProps implementation in generated files
sydneyjodon-wk Nov 28, 2023
9b2f6fa
Limit validation to late props
sydneyjodon-wk Nov 28, 2023
7a904ea
Add base test file
sydneyjodon-wk Nov 29, 2023
8d9c6ae
Add basic tests
sydneyjodon-wk Nov 30, 2023
6fed86b
Add validation escape hatches and tests
sydneyjodon-wk Dec 1, 2023
111826d
Add other boilerplate tests
sydneyjodon-wk Dec 1, 2023
7af7603
Merge branch 'null-safety' into validate-required-props-null-safety
sydneyjodon-wk Dec 1, 2023
b1ca526
Update RTL dep
sydneyjodon-wk Dec 1, 2023
880819f
Fix warning
sydneyjodon-wk Dec 1, 2023
7cdb8aa
Validate formatting on 2.19.6
sydneyjodon-wk Dec 1, 2023
10a82ce
Remove disable validation tests for backwards compatible and older bo…
sydneyjodon-wk Dec 1, 2023
0c1eabb
Merge branch 'v5_wip' into validate-required-props-null-safety
sydneyjodon-wk Dec 1, 2023
a17cee0
Fix warnings
sydneyjodon-wk Dec 1, 2023
7bb3a30
Merge branch 'v5_wip' into validate-required-props-null-safety
sydneyjodon-wk Dec 6, 2023
1c4a634
Update for props mixins only
sydneyjodon-wk Dec 6, 2023
c72755d
Fix broken tests
sydneyjodon-wk Dec 6, 2023
a3ad695
Clean up
sydneyjodon-wk Dec 6, 2023
f1c606f
Only run tests on ddc
sydneyjodon-wk Dec 6, 2023
bfcd331
Remove todos
sydneyjodon-wk Dec 6, 2023
8715d67
More cleanup
sydneyjodon-wk Dec 6, 2023
8890aad
Add multiple mixin test
sydneyjodon-wk Dec 11, 2023
c346c0c
Add back super call
sydneyjodon-wk Dec 11, 2023
1e9a5bf
Remove tests for legacy boilerplates
sydneyjodon-wk Dec 11, 2023
e362921
Merge branch 'v5_wip' into validate-required-props-null-safety
sydneyjodon-wk Dec 11, 2023
e413815
Update golds
sydneyjodon-wk Dec 11, 2023
c1c1cf2
Update lib/src/builder/codegen/accessors_generator.dart
sydneyjodon-wk Dec 15, 2023
8b9bbaa
Update lib/src/component_declaration/component_base.dart
sydneyjodon-wk Dec 15, 2023
1f0a030
Address feedback
sydneyjodon-wk Dec 15, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions lib/src/builder/codegen/accessors_generator.dart
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ abstract class TypedMapAccessorsGenerator extends BoilerplateDeclarationGenerato
}

generatedClass.write(_generateAccessors());

generatedClass.writeln('}');
generatedClass.writeln();
return generatedClass.toString();
Expand Down Expand Up @@ -149,6 +150,8 @@ abstract class TypedMapAccessorsGenerator extends BoilerplateDeclarationGenerato

StringBuffer output = StringBuffer();

final requiredPropChecks = <String>[];

node.members.whereType<FieldDeclaration>().where((field) => !field.isStatic).forEach((field) {
T? getConstantAnnotation<T>(AnnotatedNode member, String name, T value) {
return member.metadata.any((annotation) => annotation.name.name == name) ? value : null;
Expand All @@ -158,6 +161,8 @@ abstract class TypedMapAccessorsGenerator extends BoilerplateDeclarationGenerato
final requiredProp = getConstantAnnotation(field, 'requiredProp', annotations.requiredProp);
final nullableRequiredProp =
getConstantAnnotation(field, 'nullableRequiredProp', annotations.nullableRequiredProp);
final disableRequiredPropValidation = getConstantAnnotation(
field, 'disableRequiredPropValidation', annotations.disableRequiredPropValidation);

if (accessorMeta?.doNotGenerate == true) {
return;
Expand Down Expand Up @@ -198,6 +203,12 @@ abstract class TypedMapAccessorsGenerator extends BoilerplateDeclarationGenerato
annotationCount++;
isRequired = true;
isPotentiallyNullable = true;

if (type.isProps && disableRequiredPropValidation == null) {
requiredPropChecks.add(' if(!props.containsKey($keyValue)) {\n'
' throw MissingRequiredPropsError(${stringLiteral('Required prop `$accessorName` is missing.')});\n'
'}\n');
}
}

if (accessorMeta != null) {
Expand Down Expand Up @@ -352,6 +363,18 @@ abstract class TypedMapAccessorsGenerator extends BoilerplateDeclarationGenerato

output.write(staticVariablesImpl);

if (type.isProps &&
version != Version.v3_legacyDart2Only &&
version != Version.v2_legacyBackwardsCompat) {
final validateRequiredPropsMethod = '\n @override\n'
' @mustCallSuper\n'
' void validateRequiredProps() {\n'
greglittlefield-wf marked this conversation as resolved.
Show resolved Hide resolved
' super.validateRequiredProps();\n'
' ${requiredPropChecks.join('\n')}\n'
' }\n';
output.write(validateRequiredPropsMethod);
}

return output.toString();
}
}
Expand Down
6 changes: 6 additions & 0 deletions lib/src/component/abstract_transition_props.over_react.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions lib/src/component/error_boundary.over_react.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions lib/src/component/resize_sensor.over_react.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions lib/src/component/suspense_component.over_react.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions lib/src/component/with_transition.over_react.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 12 additions & 2 deletions lib/src/component_declaration/annotations.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@
// Dummy annotations that would be used by Pub code generator
library over_react.component_declaration.annotations;

// Exported for use in generated code.
export 'package:meta/meta.dart' show mustCallSuper;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting, this is so that generated code can use @mustCallSuper, right? Could we add a comment here to that effect.

Kind of gross that it has to be exported from over_react and added to its public API, though I don't think it would cause any issues...

And I can't think of any better alternatives, unless we created an alias for it that was named something else, and maybe deprecated (e.g., @Deprecated('For generated use only) const $overReact$mustCallSuper = mustCallSuper;)... Not sure if that's worth the trouble though.


/// Annotation used with the `over_react` builder to declare a `UiFactory` for a component.
///
/// @Factory()
Expand Down Expand Up @@ -274,7 +277,7 @@ class AbstractComponent2 implements AbstractComponent { // ignore: deprecated_me
///
/// Classes using this annotation must include the abstract `props` getter.
///
/// __Deprecated.__ Use the `@Props()` annotation instead if you need to make use of an annotation argument.
/// __Deprecated.__ Use the `@Props()` annotation instead if you need to make use of an annotation argument.
/// Otherwise, this can be removed completely. Will be removed in the 4.0.0 release of over_react.
@Deprecated('Use the @Props() annotation if you need to make use of an annotation argument. Otherwise, this can be removed completely. Will be removed in the 4.0.0 release of over_react.')
class PropsMixin implements TypedMap {
Expand All @@ -298,7 +301,7 @@ class PropsMixin implements TypedMap {
///
/// Classes using this annotation must include the abstract `state` getter.
///
/// __Deprecated.__ Use the `@State()` annotation instead if you need to make use of an annotation argument.
/// __Deprecated.__ Use the `@State()` annotation instead if you need to make use of an annotation argument.
/// Otherwise, this can be removed completely. Will be removed in the 4.0.0 release of over_react.
@Deprecated('Use the @State() annotation if you need to make use of an annotation argument. Otherwise, this can be removed completely. Will be removed in the 4.0.0 release of over_react.')
class StateMixin implements TypedMap {
Expand Down Expand Up @@ -381,3 +384,10 @@ class Accessor {
abstract class TypedMap {
String? get keyNamespace;
}

/// Prevents required prop validation from being performed on a prop.
const _DisableRequiredPropValidation disableRequiredPropValidation = _DisableRequiredPropValidation();

class _DisableRequiredPropValidation {
const _DisableRequiredPropValidation();
}
58 changes: 0 additions & 58 deletions lib/src/component_declaration/builder_helpers.dart
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@

library over_react.component_declaration.builder_helpers;

import 'package:meta/meta.dart';
import '../../over_react.dart';
import './component_base.dart' as component_base;
import './annotations.dart' as annotations;
Expand Down Expand Up @@ -131,63 +130,6 @@ abstract class UiProps extends component_base.UiProps with GeneratedClass {
/// This can be used to derive consumed props by usage in conjunction with [addUnconsumedProps]
/// and [addUnconsumedDomProps].
@toBeGenerated PropsMetaCollection get staticMeta => throw UngeneratedError(member: #meta);

@override
@visibleForOverriding
@mustCallSuper
void validateRequiredProps() {
super.validateRequiredProps();
// This fails when staticMeta isn't generated, so return early for now so tests don't fail.
// FIXME(null-safety) generate a static implementation of this instead in FED-1886, and remove this
return;

// ignore: dead_code
List<PropDescriptor>? missingRequiredProps;
List<PropDescriptor>? nullNonNullableRequiredProps;

for (final meta in staticMeta.all) {
for (final prop in meta.props) {
if (prop.isRequired) {
if (prop.isNullable) {
if (!props.containsKey(prop.key)) {
(missingRequiredProps ??= []).add(prop);
}
} else {
// Avoid looking up the key twice.
if (props[prop.key] == null) {
if (props.containsKey(prop.key)) {
(nullNonNullableRequiredProps ??= []).add(prop);
} else {
(missingRequiredProps ??= []).add(prop);
}
}
}
}
}
}

if (missingRequiredProps == null && nullNonNullableRequiredProps == null) {
return;
}

String formatPropKey(String propKey) => '`$propKey`';

final messageSegments = <String>[];
if (missingRequiredProps != null) {
messageSegments.add('Required props are missing: ${missingRequiredProps.map((prop) {
var propMessage = formatPropKey(prop.key);
if (prop.isNullable) propMessage += ' (can be null, but must be specified)';
return propMessage;
}).join(' ,')}.');
}
if (nullNonNullableRequiredProps != null) {
messageSegments
.add('Required non-nullable props are null: ${nullNonNullableRequiredProps.map((prop) {
return formatPropKey(prop.key);
}).join(' ,')}.');
}
throw MissingRequiredPropsError(messageSegments.join(' '));
}
}

class MissingRequiredPropsError extends Error {
Expand Down
19 changes: 16 additions & 3 deletions lib/src/component_declaration/component_base.dart
Original file line number Diff line number Diff line change
Expand Up @@ -621,9 +621,10 @@ abstract class UiProps extends MapBase

assert(_validateChildren(childArguments.length == 1 ? childArguments.single : childArguments));

// FIXME(null-safety) finalize this implementation and add escape-hatch to opt out in FED-1886
assert(() {
validateRequiredProps();
if (_shouldValidateRequiredProps) {
validateRequiredProps();
}
return true;
}());

Expand Down Expand Up @@ -676,10 +677,22 @@ abstract class UiProps extends MapBase
: const {};
}

// FIXME(null-safety) document and generate overrides in FED-1886
/// Validate at run-time that all required props are set.
///
/// This method is overridden in generated files.
@visibleForOverriding
@mustCallSuper
void validateRequiredProps() {}

/// Whether [validateRequiredProps] should be run.
var _shouldValidateRequiredProps = true;

/// Prevents [validateRequiredProps] from being called.
///
/// Allows validation to be skipped to support cases where required props are cloned onto an element.
void disableRequiredPropValidation() {
_shouldValidateRequiredProps = false;
}
}

/// A class that declares the `_map` getter shared by [PropsMapViewMixin]/[StateMapViewMixin] and [MapViewMixin].
Expand Down
13 changes: 13 additions & 0 deletions lib/src/component_declaration/flux_component.over_react.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion lib/src/over_react_redux/over_react_redux.dart
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import 'package:meta/meta.dart';
import 'package:over_react/component_base.dart';
import 'package:over_react/src/component_declaration/annotations.dart';
import 'package:over_react/src/component_declaration/builder_helpers.dart' as builder_helpers;
import 'package:over_react/src/component_declaration/builder_helpers.dart' show MissingRequiredPropsError;
import 'package:over_react/src/component_declaration/component_type_checking.dart';
import 'package:over_react/src/component_declaration/function_component.dart';
import 'package:over_react/src/util/context.dart';
Expand Down Expand Up @@ -51,7 +52,9 @@ abstract class _$ConnectPropsMixin implements UiProps {
@override
Map get props;

dynamic Function(dynamic action)? dispatch;
// Disable validation since this prop is set by the `connect` HOC, and does not need to be set by consumers.
@disableRequiredPropValidation
sydneyjodon-wk marked this conversation as resolved.
Show resolved Hide resolved
late dynamic Function(dynamic action) dispatch;
}

// ignore: prefer_generic_function_type_aliases
Expand Down
20 changes: 16 additions & 4 deletions lib/src/over_react_redux/over_react_redux.over_react.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions lib/src/util/react_util.dart
Original file line number Diff line number Diff line change
Expand Up @@ -101,4 +101,8 @@ class UiPropsMapView extends MapView

@override
void validateRequiredProps() => throw UnimplementedError('@PropsMixin instances do not implement validateRequiredProps');

@override
void disableRequiredPropValidation() =>
throw UnimplementedError('@PropsMixin instances do not implement disableRequiredPropValidation');
}

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.

Loading
Loading