-
-
Notifications
You must be signed in to change notification settings - Fork 407
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
rfc: ember-data | deprecate-non-strict-relationships #739
Merged
runspired
merged 1 commit into
emberjs:master
from
runspired:ember-data/deprecate-non-strict-relationships
Apr 13, 2022
+195
−0
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
195 changes: 195 additions & 0 deletions
195
text/0739-ember-data-deprecate-non-strict-relationships.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,195 @@ | ||
--- | ||
Stage: Accepted | ||
Start Date: 2021-04-23 | ||
Release Date: Unreleased | ||
Release Versions: | ||
ember-source: vX.Y.Z | ||
ember-data: vX.Y.Z | ||
Relevant Team(s): ember-data | ||
RFC PR: https://github.com/emberjs/rfcs/pull/739 | ||
--- | ||
|
||
# EmberData | Deprecate Non Strict Relationships | ||
|
||
## Summary | ||
|
||
Deprecates various shorthands for defining a `belongsTo` or `hasMany` | ||
relationship that create ambiguity, cause expensive runtime lookups, | ||
or hinder static analysis. | ||
|
||
## Motivation | ||
|
||
Deprecating these shorthands allows us to provide better tooling, remove the bulky | ||
and expensive code necessary to determine the outcomes of these shorthands at | ||
runtime, and in the process clarify and simplify relationship behaviors in the | ||
documentation. Further, it allows us to align our own internal usage of | ||
"relationship meta" with the API put forward in [RFC#487 Custom Model Classes](https://github.com/emberjs/rfcs/blob/master/text/0487-custom-model-classes.md#exposing-schema-information). | ||
|
||
## Detailed design | ||
|
||
Three shorthands are proposed to be deprecated, with the deprecation targeting `5.0` | ||
and being `enabled` no-sooner than `4.1`. It may be `available` sooner. | ||
|
||
- Deprecates the relationship shorthand that does not provide the related type as the first argument. | ||
|
||
This deprecation is related to deprecating `non-strict` types as it requires us to | ||
normalize the property name into a type string in a manner which may not be correct. | ||
|
||
*Before* | ||
|
||
```./before.js | ||
import Model, { belongsTo, hasMany } from '@ember-data/model'; | ||
|
||
export default class Comment extends Model { | ||
@belongsTo() | ||
author; | ||
|
||
@hasMany({ async: true, inverse: null }) | ||
likes; | ||
} | ||
``` | ||
|
||
*After* | ||
|
||
```./after.js | ||
import Model, { belongsTo, hasMany } from '@ember-data/model'; | ||
|
||
export default class Comment extends Model { | ||
@belongsTo('author') | ||
author; | ||
@hasMany('like', { async: true, inverse: null }) | ||
likes; | ||
} | ||
``` | ||
|
||
- Deprecates not explicitly setting the relationship inverse property (or `null`). | ||
|
||
The current default when not specified is to attempt to find a property on the | ||
related model that either explicity or implicitly points back, resolving to `null` | ||
if no such relationship is found and erring if more than one potential inverse was | ||
discovered. | ||
|
||
*Before* | ||
|
||
```./before.js | ||
import Model, { belongsTo, hasMany } from '@ember-data/model'; | ||
|
||
class Comment extends Model { | ||
@belongsTo('user') | ||
author; | ||
|
||
@hasMany('like') | ||
likes; | ||
} | ||
|
||
class User extends Model { | ||
@hasMany('comment') | ||
comments; | ||
} | ||
|
||
class Like extends Model { | ||
@belongsTo('user') | ||
author; | ||
} | ||
``` | ||
|
||
*After* | ||
|
||
```./after.js | ||
import Model, { belongsTo, hasMany } from '@ember-data/model'; | ||
|
||
class Comment extends Model { | ||
@belongsTo('user', { inverse: 'comments' }) | ||
author; | ||
|
||
@hasMany('like', { inverse: null }) | ||
likes; | ||
} | ||
|
||
class User extends Model { | ||
@hasMany('comment', { inverse: 'author' }) | ||
comments; | ||
} | ||
|
||
class Like extends Model { | ||
@belongsTo('user', { inverse: null }) | ||
author; | ||
} | ||
``` | ||
|
||
- Deprecates not explicitly setting `async` to `true|false`. | ||
|
||
The current default when not specified is `true`. | ||
|
||
*Before* | ||
|
||
```./before.js | ||
import Model, { belongsTo, hasMany } from '@ember-data/model'; | ||
|
||
class Comment extends Model { | ||
@belongsTo('user', { inverse: 'comments' }) | ||
author; | ||
|
||
@hasMany('like', { inverse: null }) | ||
likes; | ||
} | ||
|
||
class User extends Model { | ||
@hasMany('comment', { inverse: 'author' }) | ||
comments; | ||
} | ||
|
||
class Like extends Model { | ||
@belongsTo('user', { inverse: null }) | ||
author; | ||
} | ||
``` | ||
|
||
*After* | ||
|
||
```./after.js | ||
import Model, { belongsTo, hasMany } from '@ember-data/model'; | ||
|
||
class Comment extends Model { | ||
@belongsTo('user', { async: true, inverse: 'comments' }) | ||
author; | ||
|
||
@hasMany('like', { async: true, inverse: null }) | ||
likes; | ||
} | ||
|
||
class User extends Model { | ||
@hasMany('comment', { async: true, inverse: 'author' }) | ||
comments; | ||
} | ||
|
||
class Like extends Model { | ||
@belongsTo('user', { async: true, inverse: null }) | ||
author; | ||
} | ||
``` | ||
|
||
## Codemod | ||
|
||
A codemod should be provided. This codemod would analyze a user's relationships | ||
and wherever possible format the provided options with the now deprecated missing | ||
information. Note: For the related type and inverse property name when | ||
mixin-based polymorphism is present codemods may not be practical. | ||
|
||
## How we teach this | ||
|
||
Generally these changes improve our ability to document and explain relationship | ||
APIs as confusing behaviors (such as undefined `async` defaulting to `true`) and | ||
inverse determination are no longer magical resolutions but explicit declarations. | ||
API documentation will be updated and any examples in the guides should be as well. | ||
|
||
## Drawbacks | ||
|
||
Some churn, but via codemod we can make this quick and seamless for most apps. | ||
|
||
## Alternatives | ||
|
||
Provide new decorators that replace these. Leave these alone. While we may introduce | ||
new decorator primitives in the near future, iterating on the existing decorators to | ||
make them stricter in largely painless ways will allow more teams to migrate to a full | ||
replacement in the future with greater ease. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it vaguely odd for the before to include both strict relationships (for this
hasMany
) and non-strict (for thebelongsTo
just above)