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

Migrate to null-safety #136

Merged
merged 12 commits into from Aug 12, 2021
Merged

Migrate to null-safety #136

merged 12 commits into from Aug 12, 2021

Conversation

tiholic
Copy link
Contributor

@tiholic tiholic commented Jun 24, 2021

  • lib
  • test
  • example
  • test_integration

@github-actions github-actions bot temporarily deployed to staging/pull/136/dartdoc June 24, 2021 20:22 Inactive
@tiholic tiholic linked an issue Jun 25, 2021 that may be closed by this pull request
@ben-xD
Copy link
Contributor

ben-xD commented Jul 7, 2021

Hi @tiholic :)

Could you explain the process that you took to migrate to null safety? That would be interesting and useful to know.

I notice there a lot of usages of optionals, and I wanted to see if they're necessary (though it will require some work to find out which fields need to be optional). For example, all the fields of Message are optional, but isn't id and timestamp (and others) always known? An example of useful information for investigation is TM2.

  • There are many states that e.g. Message can be in if we make all the fields optional. This makes it complex to construct and handle.
  • Reducing the usage of optionals is useful to avoid having to unwrap optional. In fact, it might make code more verbose because optional cases need to be handled even if they do not happen in reality. This tempts the developer to use ! and just implicitly unwrap it dangerously.

Another example is AblyMessage, which is used to communicate between platform and dart. If we require (non optional) handle and type, that would make the usage of this stricter, and so on the other side of the message, we can guarantee the data to be there.

TLDR: Though we don't have compilation failures and we used the migration tool, there could be a lot of improvements because our models are too flexible, we may be abusing nullability by using ? too much.

@tiholic
Copy link
Contributor Author

tiholic commented Aug 5, 2021

Hey @ben-xD!

I followed this guide first to auto migrate https://dart.dev/null-safety/migration-guide, then looked into each of the implemented APIs (channel publish, subscribe, history, paginated results, etc). While I may also have touched few areas that are not implemented, we need to ensure they are appropriate while implementing them as per the spec.

For example, all the fields of Message are optional, but isn't id and timestamp (and others) always known?

A message can be received (read) or published (created). So fields like id and timestamp are not always expected to be present. An alternative would be to have different models for read and write (deviates from spec).

As you may have understood by now that AblyMessage.type only comes into play when we have generics involved, and handle is not always necessary (refer to usages).

As many languages do not have support for null safety, it might not have been considered while building the SDK developer docs to flag which property of a model is nullable and which is not.

Copy link
Contributor

@QuintinWillison QuintinWillison 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 very difficult to review line by line, but looks sensible to me.

@github-actions github-actions bot temporarily deployed to staging/pull/136/dartdoc August 6, 2021 19:29 Inactive
Copy link
Contributor

@ben-xD ben-xD left a comment

Choose a reason for hiding this comment

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

LGTM with comments

test_integration/test_driver/tests_abstract.dart Outdated Show resolved Hide resolved
example/lib/main.dart Outdated Show resolved Hide resolved
example/lib/nested_realtime_events.dart Show resolved Hide resolved
test/ably_event_listener_test.dart Outdated Show resolved Hide resolved
@ben-xD
Copy link
Contributor

ben-xD commented Aug 12, 2021

The failing tests have been disabled and are successfully passing (on most occasions) in #144. I'm merging this in now even though the tests fail, because the test disabling PR depends on this one.

@ben-xD ben-xD merged commit 5449f73 into main Aug 12, 2021
@ben-xD ben-xD deleted the feature/nullsafety branch August 12, 2021 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to null safety
3 participants