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-1728 Migrate to null safety #373

Merged
merged 54 commits into from
Oct 16, 2023
Merged

FED-1728 Migrate to null safety #373

merged 54 commits into from
Oct 16, 2023

Conversation

greglittlefield-wf
Copy link
Collaborator

@greglittlefield-wf greglittlefield-wf commented Oct 2, 2023

Motivation

This package needs to be upgraded to null safety in order for it to be used in other null-safe packages, and is a prerequisite for being consumed in Dart 3.

This PR contains the bulk of the changes that will make up the react-dart 7.0.0 major release.

A separate PR will be made into the v7_wip branch to perform other planned deprecated API removals. We can do a more complete audit of public API changes either at that point, or in the PR to merge the v7_wip branch.

Changes

  • Migrate to null safety
  • Additional changes that were prerequisites for null safety
    • Fix null key handling in JsBackedMap, required due to null-safe typing of getProperty/setProperty when implicit-casts are disabled, add tests
    • Update mocks to work in null safety by using mockito's builder, update CI and introduce dart_dev for formatting that excludes generated files
  • Add new asserts that callback ref args aren’t non-nullable, add tests
  • Major API changes
    • Update types to help fix implicit casts, as well as generally improve typing
      • Update ReactComponentFactoryProxy.call return type from dynamic to ReactElement
      • Update typing of top-level DOM factories from dynamic to ReactDomComponentFactoryProxy
    • API removals (more will be coming in a later PR)
      • Deprecated APIs with replacements: forwardRef, memo, main
      • All public ReducerHook and StateHook constructors
      • All public Ref constructors except for .fromJs
  • Cleanup

Testing

It can't really be made null safe, and we always intended on
removing it in the next major.
Simulated synthetic events will be missing some properties if they're
not included in eventData, meaning the non-nullable typings on them
are incorrect.

As opposed to making everything nullable to account for that, we'll
treat those simulated events the same way Mockito treats mock objects,
and just require that any properties being accessed get stubbed in using
the eventData argument.
versioning it

useRef without an argument is more common than with one, and this
approach improves that experience by:
 1. not requiring consumers explicitly provide `null` and provide
    a nullable type argument
 2. not involving a new, versioned API that must be migrated to
Importantly, this fixes some callback ref typing issues after
fixing unnecessary_lambdas lints
@greglittlefield-wf greglittlefield-wf changed the title WIP Migrate to null safety Migrate to null safety Oct 3, 2023
@greglittlefield-wf greglittlefield-wf marked this pull request as ready for review October 4, 2023 21:17
@rmconsole5-wk rmconsole5-wk changed the title Migrate to null safety FED-1728 Migrate to null safety Oct 9, 2023
Copy link
Collaborator

@aaronlademann-wf aaronlademann-wf left a comment

Choose a reason for hiding this comment

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

Didn't make it very far... will pick this back up tomorrow.

example/test/unmount_test.dart Show resolved Hide resolved
lib/hooks.dart Outdated Show resolved Hide resolved
lib/hooks.dart Show resolved Hide resolved
lib/hooks.dart Show resolved Hide resolved
lib/react.dart Outdated Show resolved Hide resolved
lib/react.dart Outdated Show resolved Hide resolved
lib/react.dart Show resolved Hide resolved
Copy link
Collaborator

@aaronlademann-wf aaronlademann-wf left a comment

Choose a reason for hiding this comment

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

This is great work!!!

CHANGELOG.md Outdated Show resolved Hide resolved
lib/react.dart Show resolved Hide resolved
lib/react.dart Show resolved Hide resolved
lib/react_client/bridge.dart Show resolved Hide resolved
lib/react_client/component_factory.dart Show resolved Hide resolved
lib/react_client/react_interop.dart Show resolved Hide resolved
lib/react_client/react_interop.dart Show resolved Hide resolved
lib/react_client/react_interop.dart Show resolved Hide resolved
expect(array is List, isTrue);
array as List;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Whoa - is this new? Cast once and be done for the remaining scope?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup! There's a bunch of tiny little type promotion improvements like this added alongside null safety; it's so nice!

componentWillUpdateWithContext/shouldComponentUpdateWithContext
Copy link
Collaborator

@aaronlademann-wf aaronlademann-wf left a comment

Choose a reason for hiding this comment

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

+1

@aaronlademann-wf
Copy link
Collaborator

+10

@greglittlefield-wf
Copy link
Collaborator Author

@Workiva/release-management-p

Copy link

@rmconsole-wf rmconsole-wf left a comment

Choose a reason for hiding this comment

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

+1 from RM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants