Advance RFC #0774 `"Deprecate Implicit Record Loading in Routes"` to Stage Recommended

[emberjs-rfcs-bot]

trafficstars

Advance #0774 to the Recommended Stage

Summary

This pull request is advancing the RFC to the Recommended Stage.

• PR to Accepted Stage: #0774
• PR to Ready For Release Stage
• PR to Released Stage

An FCP is required before merging this PR to advance.

Recommended Stage Summary

The "Recommended" stage is the final milestone for an RFC. It provides a signal to the wider community to indicate that a feature has been put through its ecosystem paces and is ready to use.

To reach the "Recommended" stage, the following should be true:

If appropriate, the feature is integrated into the tutorial and the guides prose. API documentation is polished and updates are carried through to other areas of API docs that may not directly pertain to the feature.

If the proposal replaces an existing feature, the addon ecosystem has largely updated to work with both old and new features.

If the proposal updates or replaces an existing feature, high-quality codemods are available.

If needed, Ember debugging tools as well as popular IDE support have been updated to support the feature.

If the feature is part of a suite of features that were designed to work together for best ergonomics, the other features are also ready to be "Recommended".

Any criteria for "Recommended" for this proposal that were established in the Ready For Release stage have been met.

An FCP is required to enter this stage. Multiple RFCs may be moved as a batch into "Recommended" with the same PR.

Checklist to move to Recommended

• [x] Any criteria for "Recommended" for this proposal that were established in the Ready For Release stage have been met
• [x] If appropriate, the feature is integrated into the tutorial and the guides prose. API documentation is polished and updates are carried through to other areas of API docs that may not directly pertain to the feature.
• [x] If the proposal replaces an existing feature, the addon ecosystem has largely updated to work with both old and new features.
• [ ] If the proposal updates or replaces an existing feature, high-quality codemods are available
• [x] If needed, Ember debugging tools as well as popular IDE support have been updated to support the feature.
• [x] If the feature is part of a suite of features that were designed to work together for best ergonomics, the other features are also ready to be "Recommended".
• [ ] This PR has been converted from a draft to a regular PR and the Final Comment Period label has been added to start the FCP

Criteria for moving to Recommended (required)

A set of criteria for moving this RFC to the Recommended Stage, following release:

• [x] The findModel hook raises a deprecation warning.
• [ ] Add an optional feature that opts people into the new behavior (of never automatically doing implicit record loading)
  • [x] https://github.com/emberjs/ember.js/pull/20639
  • [x] https://github.com/emberjs/ember-optional-features/pull/334
  • [ ] Document
    • [ ] Add details to deprecation guide
    • [ ] Document wherever optional features go https://github.com/ember-learn/guides-source/pull/2007
    • [x] Update docs per RFC https://github.com/ember-learn/guides-source/pull/2006
  • [ ] Add optional feature to CLI blueprint (defaulted to false) https://github.com/ember-cli/ember-cli/pull/10440
  • [ ] Remove optional feature at 6.0
  • [ ] Update release versions/dates on RFC
  • [ ] Release @ember/optional-features

[wagenet]

This should probably advance since it's a deprecation and there's no further work to do here.

[achambers]

If the proposal updates or replaces an existing feature, high-quality codemods are available

Is this something we should be considering? After discussing we were unconvinced that it'd be worth the effort of creating a codemod for this. Thoughts?

[wagenet]

I don't feel like we need codemods for this, but I'm also not opposed.

[jelhan]

In addition, we will include an optional feature to disable this feature and clear the deprecation.

Has this optional feature been implemented? I don't see one in config/optional-features.json on ember-new-output: https://github.com/ember-cli/ember-new-output/blob/042bd03c165de8083e9ea9ab4b21eae5c7cffdd1/config/optional-features.json

[achambers]

In addition, we will include an optional feature to disable this feature and clear the deprecation.

Has this optional feature been implemented? I don't see one in config/optional-features.json on ember-new-output: ember-cli/ember-new-output@042bd03/config/optional-features.json

This is not the kind of change we use optional features for. We have the deprecation workflow for this purpose. The RFC should probably be edited to reflect this instead.

[jelhan]

This is not the kind of change we use optional features for. We have the deprecation workflow for this purpose. The RFC should probably be edited to reflect this instead.

Not sure if I agree. Without an optional feature you must implement a model hook to opt-out of implicit record loading. You end up with empty model hooks, which are confusing to everyone not being aware of this deprecated feature.

[ef4]

The RFC text calls for an optional feature for deliberate reasons. Without the optional feature, users who are not relying on the deprecated behavior have no way of removing the warning without writing temporary code that wouldn't actually be needed in the upcoming ember major version.

There are legitimate use cases that are safe on the next Ember major that cause deprecations warnings. The optional feature lets you skip to the new implementation without waiting for the major, avoiding the need to introduce temporary code just to convince Ember your code is safe.

So I agree with @jelhan's catch and this isn't ready to move to recommended with the missing optional feature.

[ef4]

Status update here: we still need someone to implement the optional feature that opts people into the new behavior (of never automatically doing implicit record loading).

[achambers]

Here's a draft PR for the optional-features repo: https://github.com/emberjs/ember-optional-features/pull/334

Was working on this with @kategengler . She will have a PR for the ember.js repo.

[bertdeblock]

Regarding the name of the new optional feature. I think implicit-route-model: true|false would be more consistent with the rest, and easier to understand. The double negative (not sure if that's how you say it) is harder to read and understand I feel => no-implicit-route-model: false.

[kategengler]

Regarding the name of the new optional feature. I think implicit-route-model: true|false would be more consistent with the rest, and easier to understand. The double negative (not sure if that's how you say it) is harder to read and understand I feel => no-implicit-route-model: false.

We thought about this when we added it -- felt it was easier to have the "on" value as a true ... so to have the existing behavior the flag is off with no-implicit-route-model: false (indeed, a "double negative"), but to proactively remove the behavior as it would be done in 6.0, it is no-implicit-route-model: true

[achambers]

Thoughts on @mansona 's comment https://github.com/ember-cli/ember-cli/pull/10440#discussion_r1489677310 ?

[mansona]

Just in case anyone is following along here and didn't see my other comment here is what I'm proposing:

I feel like we should be defaulting to the new (desired) behaviour [right away] when generating a new app with [the default app blueprint] 🤔

[...] considering we are hoping to remove this code in 6.0 it seems reasonable for new apps generated between now and then to be generated with the new stuff

Does anyone object to introducing this optional feature with the new state? i.e. add

"no-implicit-route-model": true

to the optional-features.json

[kategengler]

Just in case anyone is following along here and didn't see my other comment here is what I'm proposing:

I feel like we should be defaulting to the new (desired) behaviour [right away] when generating a new app with [the default app blueprint] 🤔 [...] considering we are hoping to remove this code in 6.0 it seems reasonable for new apps generated between now and then to be generated with the new stuff

Does anyone object to introducing this optional feature with the new state? i.e. add

"no-implicit-route-model": true

to the optional-features.json

I agree with this; I think we should update the RFC to say the optional feature should be immediately enabled in the blueprint so that new apps do not get this old behavior, especially as it will be removed soon in 6.0.

[achambers]

Just in case anyone is following along here and didn't see my other comment here is what I'm proposing:

I feel like we should be defaulting to the new (desired) behaviour [right away] when generating a new app with [the default app blueprint] 🤔 [...] considering we are hoping to remove this code in 6.0 it seems reasonable for new apps generated between now and then to be generated with the new stuff

Does anyone object to introducing this optional feature with the new state? i.e. add "no-implicit-route-model": true to the optional-features.json

I agree with this; I think we should update the RFC to say the optional feature should be immediately enabled in the blueprint so that new apps do not get this old behavior, especially as it will be removed soon in 6.0.

RFC Update here: https://github.com/emberjs/rfcs/pull/1010