Fixed null-safety issues #30

dukefirehawk · 2021-10-19T15:32:09Z

Changed parameters of component constructor with @Attribute annotation to be nullable
Fixed onPush issue
Fixed null-safety issues in galleries

GZGavinZhao · 2021-10-20T02:08:49Z

Does it build? On my computer it doesn't.

Dart SDK version: 2.14.4 (stable) (Wed Oct 13 11:11:32 2021 +0200) on "linux_x64"

dukefirehawk · 2021-10-20T06:28:44Z

Hold on first. I need to check in a a few more null-safety fixed to clear away those errors.

GZGavinZhao · 2021-10-21T21:29:06Z

@dukefirehawk No problem! Tell me when you're ready. ~~I will push the Sass fix tonight.~~Done, no Sass warnings should produce now.

lejard-h · 2021-10-23T11:35:06Z

Next time can we address issues one by one in multiple PR.

Reviewing such PR is very hard.

For OnPush issue, we don't know if just changing the change detection strategy is engouh, maybe we need to call markForCheck() sometimes.

lejard-h

Is it an automated null safety migration ?

I see a lot of @Input being nullable for no obvious reason.

Also is @Inject() annotation is not able to get Type anymore ? Why some types has been replaced by Object ?

lejard-h · 2021-10-23T12:08:25Z

angular_components/lib/focus/focus.dart

@@ -231,8 +231,8 @@ class AutoFocusDirective extends RootFocusable implements OnInit, OnDestroy {
  /// This value should not change during the component's life.
  // TODO(google): Change to an attribute.
  @Input()
-  set autoFocus(bool value) {
-    _autoFocus = value;
+  set autoFocus(bool? value) {


maybe we could force it to be non null first. so we don't need to check for nullability and set it to false

None nullable fields need to be initialized. They are automatically converted during migration. Agree that it is not a good choice but then most if not all components need to have their fields initialized by going non nullable.

lejard-h · 2021-10-23T12:09:27Z

angular_components/lib/laminate/overlay/module.dart

@@ -31,8 +30,11 @@ export 'package:angular_components/src/laminate/overlay/render/overlay_dom_rende
 /// already.
 /// A hidden focusable element is inserted before and after the overlay
 /// container to support a11y features.
-HtmlElement createAcxOverlayContainer(HtmlElement parent,
+HtmlElement createAcxOverlayContainer(Object parent,


why not keeping HtmlElement here ?

There are dependency across the codebase. Fixing one issue often trigger a chain reaction. The change to Object due to generated code in template.dart does not compile with HtmlElement.

Probably because of this #36

lejard-h · 2021-10-23T12:10:44Z

angular_components/lib/laminate/overlay/module.dart

-    @Inject(overlayContainerParent) HtmlElement parent,
-    @Optional() @SkipSelf() @Inject(overlayContainerToken) container) {
-  if (container != null) return container;
+    @Inject(overlayContainerName) Object name,


Same here, using Object is strange. You also cast it after

lejard-h · 2021-10-23T12:11:18Z

angular_components/lib/laminate/overlay/module.dart

+    @Optional()
+    @SkipSelf()
+    @Inject(overlayContainerName)
+        Object? containerName) {


Template.dart will fail to compile complaining String? is not Object?

lejard-h · 2021-10-23T12:13:16Z

angular_components/lib/button_decorator/button_decorator.dart

@@ -55,39 +55,40 @@ class ButtonDirective extends RootFocusable

  /// String value to be passed to aria-disabled.
  @HostBinding('attr.aria-disabled')
-  String get disabledStr => '$disabled';
+  String? get disabledStr => '$disabled';


I don't see why it can be nullable, we always return a String

@lejard-h @GZGavinZhao Not much time to get back for the past few days. The bulk of these changes are done to address issues flagged up in the gallery examples build stage. I will share some of the findings and the fixes that are put in.

Issue The argument type 'Object' can't be assigned to the parameter type xxx #36 @lejard-h fix is a better way.

Contructor marked as @optional must be nullable. Change to use nullable type. Need to make changes from parent class all way to the base abstract class.

Constructor not marked with @optional must be non nullable. Similar to 3 above.

Most of the components including class have fields declared without initialization. Add initializer wherever possible. Field holding function with generic type is problematic. Either late or make it nullable.

Some factory class explicitly returns null. Return a custom empty class or throw ArguemetError.

Some fields are explicitly set to null to clear the conditions or status. Remove set to null.

Logic with List, Map or Iterable null checking in the code. Change to isEmpty check. Default List and Iterable to [] and map to {}

Updating dom only accepts string type. This is an odd one. Will share the details once i nailed the flow down.

@input fail when type in non nullable. Make it nullable on case by case basis. This one usually need to be relooked at. @lejard-h has pointed out some of these.

@lejard-h @GZGavinZhao Take this case for example, the generated code in .template.dart file for @HostbBinding is

final currVal_2 = this.instance.disabledStr; if (import4.checkBinding(this._expr_2, currVal_2, null, null)) { import5.updateAttribute(el, 'aria-disabled', currVal_2); this._expr_2 = currVal_2; }

It is calling dom_helpers.updateAttribute in Angular 7. Passing a nullable type should produce the right behaviour, otherwise the attribute would never be removed. Also noticed that it only works with String type. So fields with @HostBinding annotated with other types will fail. This is issue 8 in my list above.

/// Updates [attribute] on [element] to reflect [value]. /// /// If [value] is `null`, this implicitly _removes_ [attribute] from [element]. @dart2js.noInline void updateAttribute( Element element, String attribute, String? value, ) { if (value == null) { element.removeAttribute(attribute); } else { setAttribute(element, attribute, value); } domRootRendererIsDirty = true; }

Yes Optional() injection must be nullable but not Object?

for disableStr, maybe it should be String? get disabledStr => disabled ? 'true' : null; ?

That works. Object? is just an interim fix for running debugger through the builder process to see what type goes where. They will be replacement once the actual type has been figured out.

lejard-h · 2021-10-23T12:15:30Z

angular_components/lib/material_checkbox/material_checkbox.dart

@@ -120,29 +122,29 @@ class MaterialCheckboxComponent
  ///
  /// `true` will go to checked and `false` will go to unchecked.
  @Input()
-  bool indeterminateToChecked = false;
+  bool? indeterminateToChecked = false;


any reason to make it nullable ?

lejard-h · 2021-10-23T12:21:01Z

Also seeing a lot of commented code, is it ready for review ?

lejard-h · 2021-10-23T17:29:53Z

I think you guys went too fast on null safety migration, the plan was to make the package compatible with Angular 7 First. I also probably don't have all the infos of what you already done.

Now the package is compatible with Angular 7, can we coordinate here #35 to progressively migrate the package layer by layer.

I suggest we create a new branch null-safety base on master (after this PR #32) then we partially merge the work already done on dev branch following the TODO list in the issue.

I also think changing type to Object is not a good idea, we probably missed something.

GZGavinZhao · 2021-10-29T16:44:29Z

@dukefirehawk Please tell me when you're ready. :)

I recommend not trying to migrate new areas and finish with what you have now. We want to merge this into null-safety branch, which already has some fixes, especially for the deprecated API stuff.

dukefirehawk · 2021-10-31T00:20:17Z

@GZGavinZhao @lejard-h This PR can build and compile everything without errors now. However, the examples has runtime issues that need to be addressed. I will also refactor some of the ugly fix later but it would be done as separate small PR. Take a look at the changes. To fix [attr.xxx]="someBool", I have to introduce attributeToString(someBool) into the html code instead of creating a method someBoolStr and do [attr.xxx]="someBoolStr". Let me know if you have better suggestion for this. As mentioned the underlying Angular method only accepts "String?", so every other type except string must be changed for attr.xxx field to bind to string.

dukefirehawk · 2021-11-02T03:37:21Z

Only the following examples still need some fixing, the rests are working.
Material Auto Suggestion
Material Date Picker
Material Dialog
Material Dropdown Select
Material Expansion Panel
Material Input
Material Menu
Material Popup
Material Select
Material Tab
Material Tooltip
Material Tree

jodinathan · 2021-11-08T13:52:02Z

please, can you enlighten me regarding this PR?
I mean, does it make the whole repo null safe? Do you need help?

dukefirehawk · 2021-11-08T17:44:25Z

@jodinathan This PR brings the whole repo to null safety. Current status is can compile and build everything successful in null safety mode. However, there are still runtime errors in those listed components related to incompatible function types, dynamic type casting and logic that is based on null to work. These errors show up in browser console when clicking through the examples.

GZGavinZhao · 2021-11-08T22:34:45Z

Shall I merge what we have now first? @dukefirehawk can list out the parts that are not working, then we can update its status with #35 and address the problems/bugs one by one. This PR is getting huge.

GZGavinZhao · 2021-11-08T22:59:54Z

@dukefirehawk I just generated a much more comprehensive and powerful CI script. You can pull that to your repo and perhaps get a lot more feedback from it than the useless 30-line script I wrote. :)

dukefirehawk · 2021-11-09T01:14:33Z

@GZGavinZhao I've checked in the latest fix. The CI is good but seems to have failed at install_protoc.sh. Can you take a look at it? Otherwise, the code can build and run in null safety mode. What's left are running through the examples (mostly working) and fixing the issues (Usually exception with the class and line number of the code displayed in the browser console). Only Material Tree is still pretty broken as incompatible types are being used. If someone want to take a crack at it, let me know.

EXCEPTION: Expected a value of type 'Parent<dynamic, Iterable<OptionGroup<dynamic>>>', but got one of type '_NestedSelectionOptions<dynamic>', package:angular_components/src/material_tree/material_tree_node.dart 52

Further refactoring can also to be done to get rid of ? for Input field but need to be careful as some of them use null check to switch logic.

dukefirehawk · 2021-11-10T02:32:51Z

@GZGavinZhao Your fix on proto works.

jodinathan · 2021-11-10T22:40:42Z

isn't the compatible branch merged into this?
I am testing and it is complaining about the ChangeDetection

dukefirehawk · 2021-11-10T23:26:40Z

@jodinathan Which component has the issue?

jodinathan · 2021-11-10T23:27:54Z

material-tab, material-auto-suggest-input.

dukefirehawk · 2021-11-11T01:06:02Z

As mentioned, those two components still have some issues. For material-auto-suggest-input, it uses material popup internally which has some logic that seems to suggest that it requires null to work. Once the underlying type becomes non nullable, it just broke. Working on them at the moment. Will use smaller individual PR per component for the change going forward.

GZGavinZhao · 2021-11-11T01:23:39Z

@dukefirehawk Can you fill out #35 in the meantime?

jodinathan · 2021-11-11T10:47:53Z

As mentioned, those two components still have some issues. For material-auto-suggest-input, it uses material popup internally which has some logic that seems to suggest that it requires null to work. Once the underlying type becomes non nullable, it just broke. Working on them at the moment. Will use smaller individual PR per component for the change going forward.

Out of curiosity... When I fixed the compatible branch the auto-suggest-input was already at OnPush, so why isn't this fixed at this branch?
And why isn't the compatible branch merged into this one?

dukefirehawk · 2021-11-11T13:51:31Z

dev branch was originally forked from master together along with compatibility branch. It was my bad to jump start the null safety migration a bit too early due to my need of a few components. By the time, some of the fixes went into compatibility branch, null safety work on dev branch has progressed into compiling the examples. I just finished up on what was started instead of left it hanging in a non compilable state. I believe null-safety branch was created with the intention to cherry pick and merge dev branch into compatible branch.

dukefirehawk added 5 commits October 18, 2021 13:16

Fixed onPush issue

43cb83a

Fixed incorrect attributes

44d721f

Updated displayNameRendererDirective

53c5495

Fixed null-safety issues

8926804

Update changelog

079e91d

GZGavinZhao added the null safety Related to null safety migration label Oct 20, 2021

Fixed null-safety issues

46a6625

dukefirehawk added 4 commits October 23, 2021 09:12

Reverted deprecated componentType

2e334e0

Fixed menu example

dce2468

Merge branch 'dev' into migrated/bug-fix-1

79210c8

Merge branch 'migrated/bug-fix-2' into migrated/bug-fix-1

f576bbe

lejard-h reviewed Oct 23, 2021

View reviewed changes

Fixed all build issues in angular_gallery_section

ea9c2ed

GZGavinZhao mentioned this pull request Oct 27, 2021

Null safety migration #35

Open

54 tasks

Fixed type issues with HostBinding and 'attr.xxx`

a2f5aa9

dukefirehawk added 2 commits October 30, 2021 23:47

Fixed null safety in examples

60b5902

Fixed null safety issues in examples

d592e60

dukefirehawk added 2 commits November 2, 2021 10:51

Replaced DomSanitizer with sanitize_html

852564f

Fixed late initializer

20f2f48

Fixed material input

b94a123

dukefirehawk added 4 commits November 3, 2021 11:40

Fixed popup

f798011

Removed deprecated

9ca4740

Fixed material tab

691786c

Removed deprecated componentType

a8379c3

Refactored NNBD in material tree

1a1ef5d

dukefirehawk added 5 commits November 9, 2021 07:18

Merge branch 'dev' into migrated/bug-fix-1

9f375b3

Fixed display name in material select

1aa827b

Fixed CI warnings

06e451d

Fixed CI warning

2d879fe

Fixed CI warning

8a965db

Fixed CI proto warning

a968fe9

GZGavinZhao mentioned this pull request Nov 10, 2021

Fix protoc generation #57

Closed

dukefirehawk merged commit ac7b0eb into angulardart-community:dev Nov 10, 2021

dukefirehawk deleted the migrated/bug-fix-1 branch December 3, 2022 04:32

Fixed null-safety issues #30

Fixed null-safety issues #30

Conversation

dukefirehawk commented Oct 19, 2021

GZGavinZhao commented Oct 20, 2021

dukefirehawk commented Oct 20, 2021

GZGavinZhao commented Oct 21, 2021 • edited Loading

lejard-h commented Oct 23, 2021

lejard-h left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dukefirehawk Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dukefirehawk Oct 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lejard-h commented Oct 23, 2021

lejard-h commented Oct 23, 2021 • edited Loading

GZGavinZhao commented Oct 29, 2021 • edited Loading

dukefirehawk commented Oct 31, 2021 • edited Loading

dukefirehawk commented Nov 2, 2021

jodinathan commented Nov 8, 2021

dukefirehawk commented Nov 8, 2021

GZGavinZhao commented Nov 8, 2021

GZGavinZhao commented Nov 8, 2021 • edited Loading

dukefirehawk commented Nov 9, 2021

dukefirehawk commented Nov 10, 2021

jodinathan commented Nov 10, 2021

dukefirehawk commented Nov 10, 2021

jodinathan commented Nov 10, 2021

dukefirehawk commented Nov 11, 2021

GZGavinZhao commented Nov 11, 2021

jodinathan commented Nov 11, 2021

dukefirehawk commented Nov 11, 2021

GZGavinZhao commented Oct 21, 2021 •

edited

Loading

dukefirehawk Oct 25, 2021 •

edited

Loading

dukefirehawk Oct 27, 2021 •

edited

Loading

lejard-h commented Oct 23, 2021 •

edited

Loading

GZGavinZhao commented Oct 29, 2021 •

edited

Loading

dukefirehawk commented Oct 31, 2021 •

edited

Loading

GZGavinZhao commented Nov 8, 2021 •

edited

Loading