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-1713 Null safety: perform migration, set up base for 5.0.0 integration branch #856

Merged
merged 219 commits into from
Dec 1, 2023

Conversation

greglittlefield-wf
Copy link
Contributor

@greglittlefield-wf greglittlefield-wf commented Nov 14, 2023

Motivation

We need to migrate over_react to null safety.

This migration has been spiked and developed intermittently within the null-safety branch, and it's time to get that work cleaned up and merged into an integration branch, to use as a base to complete the remaining null safety work.

Changes

  • Initial implementations of updating codegen to support null-safe libraries:
    • Generating null-safe code, conditionally based on source library (to be completed in FED-1720)
    • Ability to specify non-nullable late props (to be completed in FED-1717)
  • Migrate package to null safety
  • Miscellaneous notable changes
    • Add WIP changelog entry
    • Clean up noisy test output by declaring test tags in dart_test.yaml and suppressing some printed output
    • Updated getPropKey implementation to work with non-nullable props since they throw when not specified
  • Breaking changes
    • Made components with handwritten props classes into a uiJsComponents with generated props classes
      • Fragment
      • StrictMode
      • ReduxProvider (also added ReduxProviderPropsMixin)
    • PropsMeta/StateMeta constructor arguments fields and keys are now required
  • Analyzer plugin partial implementations
    • MissingRequiredPropDiagnostic support for late props spikes (to be completed in FED-1886)
    • Sound null safety in tests, re-enable previously-unsound diagnostics (to be completed in FED-1763)

Review notes:

This is a pretty massive diff. I'd recommend reviewing it in the chunks listed below.

For review feedback that requires more extensive investigation or changes, let's lean toward cutting tickets and adding FIXMEs so that we can get these changes merged sooner.

If there are clean diffs or anything else I can set up to make the reviewing process more manageable, please let me know!

Recommended chunks (in whichever order you prefer):

  • Builder, builder tests
    • lib/src/builder/
    • test/vm_tests/builder/
    • test_fixtures/
  • Utility components Review instead in FED-1889 Null Safety: Audit public utility components #855
    • lib/src/component/_deprecated/
    • lib/src/component/abstract_transition*
    • lib/src/component/error_boundary*
    • lib/src/component/resize_sensor*
    • lib/src/component/with_transition.dart
    • lib/src/util/safe_render_manager/
  • Flux
    • lib/src/component_declaration/flux_component
  • OverReact Redux
    • lib/src/over_react_redux
  • Component core and utilities
    • lib/src/component/ (minus utility components)
    • lib/src/component_declaration/ (minus Flux)
    • lib/src/util/
  • Analyzer plugin
    • tools/analyzer_plugin/
  • Examples
    • example/
    • web/

QA Checklist

  • Tests were updated and provide good coverage of the changeset and other affected code
  • Manual testing was performed if needed
    • Steps from PR author:
      • CI passes (except analyzer plugin; those will be fixed in follow-up PRs)
    • Anything falling under manual testing criteria outlined in CONTRIBUTING.md

Merge Checklist

While we perform many automated checks before auto-merging, some manual checks are needed:

  • A Frontend Frameworks Design member has reviewed these changes
  • There are no unaddressed comments - this check can be automated if reviewers use the "Request Changes" feature
  • For release PRs - Version metadata in Rosie comment is correct

aaronlademann-wf and others added 30 commits February 4, 2021 16:01
+ Now that there are tagged releases that work with 2.12 and analyzer 0.40.x
…hints but not applying the migration

Also excludes:
- lib/src/component
- lib/src/over_react_redux/devtools/
- lib/src/over_react_redux/over_react_flux.dart
Copy link
Contributor

@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.

Screenshot 2023-11-27 at 1 09 56 PM

wow

Copy link
Contributor

@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

@greglittlefield-wf
Copy link
Contributor Author

@aaronlademann-wf left a comment

+1

David from Schitt's Creek clapping happily and saying "Thank you"

David from Schitt's Creek smiling and saying "You're a godsend"

@greglittlefield-wf greglittlefield-wf merged commit 8eeb5cd into v5_wip Dec 1, 2023
7 of 10 checks passed
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