deprecate App::data and App::data_factory

[robjtede]

PR Type

Deprecation

PR Checklist

• Tests for the changes have been added / updated.
• Documentation comments have been added / updated.
• A changelog entry has been made for the appropriate packages.
• Format code with the latest stable rustfmt.
• (Team) Label with affected crates and semver status.

Overview

Partially addresses the problem we have with .app_data(val) vs .data(val) vs .app_data(Data::new(val)) by deprecating .data(val) and encouraging use of .app_data(Data::new(val)) as a replacement.

Also deprecates App::data_factory() because of it's limited use. There is a plan to introduce a similar type LazyData post-v4 that would be used in the same way as Data and more useful because it allows the app to start serving requests before it resolves.

[ibraheemdev]

Won't people just start doing .app_data(Foo)?

[robjtede]

Won't people just start doing .app_data(Foo)?

Hopefully the deprecation notice will accurately guide existing users to the new code setup.

For new users, I suspect it will be far easier to document on .app_data and the Data extractor how they work together without having to talk about 2 methods for inserting data values. This should become even clearer when there is more than 1 data extraction type.

I think there are more steps we can take to help, too. For example, the Data<T> extraction debug error could check the inner T type's existence in the map and report it in the error with instructions on how to fix it.

Very willing to debate this further and hash out the complete set of implications; consider this my opening statement.

[popzxc]

I'm OK with changes and actually it makes sense and looks good.

However, now tests for these methods have warnings, and due to many jobs it makes the diff bloated and hard to watch. Wouldn't it be better to add #[allow(deprecated)] to these tests?

[rsalmei]

Since this will be a major version release, why don't you just remove instead of deprecate?

[robjtede]

Since this will be a major version release, why don't you just remove instead of deprecate?

It would be overly distruptive to migration since it would affect almost all AW apps.

[otavio]

I second the removal as well. We kind of expect breaking changes when doing major upgrades so it allows the removal of old code.

We could mark it as deprecated if releasing it on a minor or patch versions.

[popzxc]

Nah, I think that it'd be too radical. While I'm all in for removing things, but for a mature framework that is already being used in production by many teams, it's better to do things gradually.

It's OK to clean up the code, but it's less OK to be not predictable and sudden.

Given the fact that 4.0 is not so far away, it'll reduce amount of people who will be able to quickly switch to a new version.