search

emberjs / rfcs

RFCs for changes to Ember
https://rfcs.emberjs.com/
791 stars 406 forks source link

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

Closed emberjs-rfcs-bot closed 8 months ago

emberjs-rfcs-bot commented 1 year ago

Advance #0774 to the Recommended Stage

Summary

This pull request is advancing the RFC to the Recommended 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

Criteria for moving to Recommended (required)

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

wagenet commented 1 year ago

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

achambers commented 1 year ago

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 commented 1 year ago

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

jelhan commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 10 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

mansona commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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