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-2040 Placeholder null safety migration guide #910

Merged
merged 8 commits into from
Apr 18, 2024
Merged
Changes from 3 commits
Commits
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
81 changes: 81 additions & 0 deletions doc/null_safety/null_safe_migration.md
greglittlefield-wf marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
# Dart Null Safety Migration Guide

Steps to migrate a repo to null safety:
greglittlefield-wf marked this conversation as resolved.
Show resolved Hide resolved

1. [View resources](#step-1-view-resources)
1. [Run the preparation codemod](#step-2-run-the-preparation-codemod-coming-soon)
1. [Run the migration tool](#step-3-run-the-migration-tool)

## Step 1: View resources

If you haven't already, familiarize yourself with the concepts of Dart null safety with the following resources:

- [Understanding null safety](https://dart.dev/null-safety/understanding-null-safety) article

<!-- TODO - add links to over_react null safety documentation once it exists -->

## Step 2: Run the preparation codemod (coming soon!)

<!-- TODO - update this section once we release the `null_safety_prep` codemod. -->

> ⚠️ This codemod does not exist yet, but is coming soon!

Start out by running the `null_safety_prep` [codemod][orcm] on your repo:

```
dart pub global activate over_react_codemod
dart pub global run over_react_codemod:null_safety_prep
```

This codemod will:
- Migrate as many over_react specific use cases as possible.
- Get the repo into a state where the migration tool can be run with less manual intervention.

## Step 3: Run the migration tool

Run the null safety migrator tool:

- For Workiva employees, use the [Workiva version of the migrator tool](https://github.com/Workiva/wnnbd?tab=readme-ov-file#migrating-a-package).
- Otherwise, use [Dart's migrator tool](https://dart.dev/null-safety/migration-guide#migration-tool).

### Common migration cases

Below are some common cases that might come up while running the migrator tool on a repo using over_react.

#### Prop nullability

To determine if a prop should be nullable or not, first consider if the prop is required.

<!-- TODO - do we need to list a reasons props could be required?? If a prop is defaulted, does that make it required too? -->
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll go into more depth about this in my null safe props PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay perfect! Thank you!


> ⚠️ Making a prop required with the `late` keyword can be a breaking change if consumers are not always setting the prop.
Copy link
Contributor

Choose a reason for hiding this comment

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

#fyi (what you have is fine) there's a built-in way to do warning callouts with "alerts":

Suggested change
> ⚠️ Making a prop required with the `late` keyword can be a breaking change if consumers are not always setting the prop.
> [!WARNING]
> Making a prop required with the `late` keyword can be a breaking change if consumers are not always setting the prop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that's awesome!! Thank you!


Below is a table of the possible options for prop nullability:

| | Required (`late`) | Optional |
|----------------|----------------------|-----------------|
| Nullable (`?`) | `late String? prop;` | `String? prop;` |
| Non-nullable | `late String prop;` | n/a |

- (_most common_) All **optional** props should be made **nullable**.
- **Required** props can be nullable or non-nullable:
- **Nullable**: If the prop is required, but can be explicitly set to `null`.
- **Non-nullable**: If the prop is required and should never be set to `null`.

#### Potential Gotchas

The migrator tool [incorrectly treats the result of emulated function calls as nullable](https://github.com/dart-lang/sdk/issues/46263).

The most common way this affects over_react is in cases like this:
```dart
// The over_react code:
var content = Dom.div()();

// gets incorrectly migrated to:
ReactElement? content = Dom.div()();
```

<!-- TODO - not really sure how to explain this or if there is a solution besides manual overriding?
Is this fixed in the Workiva version of the tool that we should link to? -->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@greglittlefield-wf do you have any thoughts on this one?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question; we could add instructions for running the migrator tool with the fix in my branch/CL that's linked in that Dart issue.

It's a little involved since it involves cloning and partially setting up the Dart SDK, so how about I write that up and PR that separately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be great! Thank you!


[orcm]: https://github.com/Workiva/over_react_codemod
Loading