-
-
Notifications
You must be signed in to change notification settings - Fork 408
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
EmberData | deprecate Store extends EmberObject #1026
Merged
Merged
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
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
109 changes: 109 additions & 0 deletions
109
text/1026-ember-data-deprecate-store-extends-ember-object.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,109 @@ | ||
--- | ||
stage: accepted | ||
start-date: 2024-05-11T00:00:00.000Z | ||
release-date: # In format YYYY-MM-DDT00:00:00.000Z | ||
release-versions: | ||
teams: # delete teams that aren't relevant | ||
- data | ||
prs: | ||
accepted: https://github.com/emberjs/rfcs/pull/1026 | ||
project-link: | ||
suite: | ||
--- | ||
|
||
# EmberData | Deprecate Store extending EmberObject | ||
|
||
## Summary | ||
|
||
This RFC deprecates the Store extending from EmberObject. All EmberObject specific | ||
APIs included. | ||
|
||
## Motivation | ||
|
||
There are two motivations: | ||
|
||
First, extending EmberObject is vestigial. The Store makes no use of any EmberObject API, | ||
not even for use with Ember's container or service injection. | ||
|
||
Second, in order to support any Ember version, support any non-Ember framework, and support | ||
EmberData running in non-browser environments we want to remove unnecessary coupling to the Ember framework. | ||
|
||
## Detailed design | ||
|
||
Instead of deprecating every EmberObject method, we will feature flag the Store extending | ||
EmberObject at the module level. This ensures the deprecation only prints once, and that | ||
once resolved the Store will no longer extend thereby making it feasible to utilize the | ||
benefits of not extending EmberObject immediately. | ||
|
||
To resolve the deprecation, users will need to confirm they are not using EmberObject APIs | ||
on the Store. Generally speaking, this has been limited to `.extend` e.g. | ||
|
||
```ts | ||
const AppStore = Store.extend({}); | ||
``` | ||
|
||
This pattern is now rare in the wild, but where it exists can be safely refactored to | ||
|
||
```ts | ||
class AppStore extends Store {} | ||
``` | ||
|
||
Once confirmed (or in order to confirm) that the Store in an app no longer requires | ||
extending EmberObject, the deprecation config boolean may be used to both remove the | ||
deprecation AND the deprecated code. | ||
|
||
```ts | ||
const app = new EmberApp(defaults, { | ||
emberData: { | ||
deprecations: { | ||
DEPRECATE_STORE_EXTENDS_EMBER_OBJECT: false | ||
} | ||
} | ||
}); | ||
``` | ||
|
||
An upcoming shift in how EmberData manages configuration would mean that applications | ||
using the new configuration (not yet released) would do the following: | ||
|
||
```ts | ||
'use strict'; | ||
|
||
const EmberApp = require('ember-cli/lib/broccoli/ember-app'); | ||
|
||
module.exports = async function (defaults) { | ||
const { setConfig } = await import('@warp-drive/build-config'); | ||
|
||
const app = new EmberApp(defaults, {}); | ||
|
||
setConfig(app, __dirname, { | ||
deprecations: { | ||
DEPRECATE_STORE_EXTENDS_EMBER_OBJECT: false | ||
} | ||
}); | ||
|
||
return app.toTree(); | ||
}; | ||
``` | ||
|
||
## How we teach this | ||
|
||
Guides would be added for this deprecation to both the deprecation app and the API docs. | ||
|
||
Generally, folks do not tend to treat the Store as an EmberObject or utilize legacy EmberObject | ||
APIs with it, so both the teaching and the migration overhead are low. | ||
|
||
## Drawbacks | ||
|
||
none | ||
|
||
## Alternatives | ||
|
||
- deprecate every classic method to help folks find usage | ||
- not chosen as it's rare *and* setting the deprecation flag to `false` will cause any such locations to be findable via error | ||
- create a new package `@warp-drive/core` or `@warp-drive/store` and have users migrate by swapping import | ||
locations. | ||
- not chosen as this is too minimal a change | ||
|
||
## Unresolved questions | ||
|
||
None |
Oops, something went wrong.
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.
We've had a bit of a conversation on this in RFC review, I don't know if I'm overthining things but have we considered any of the negative aspects of introducing a new way to turn off deprecations like this?
i.e. we recently rolled out the deprecation for the implicit root model which requires you to update an optional feature to turn off the deprecation.
I feel like having two places to explain and document might get a bit strange, but I don't know if I'm over-indexing on how bad that would be 🤔
I'm also concerned that that introducing things like this into the ember-cli-build file doesn't feel very modern in terms of the modern tooling we're working on (Embroider, Vite) and I think the more natural way to achieve the desired outcome would be to either change the import location (as mentioned in the alternatives) or changing the thing that is imported e.g.
import { NewStore as Store } from '@ember-data/store'
On that point what does
not chosen as this is too minimal a change
mean? 😂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.
This is in fact not a new way! This has been the way to resolve deprecations and remove associated code in EmberData for a few years now. This docs for this are here: https://api.emberjs.com/ember-data/release/modules/@ember-data%2Fdeprecations
EmberData/WarpDrive is not ember-source and its pretty hard for us to piggy back on ember-source infra, especially without introducing accidental coupling we don't want.
Just the opposite! Our config story was already driven by macros, and in preparation for stand-alone (no-embroider) vite and v2-addons we've extracted that into a
setConfig
function. In ember apps, the most natural place to invoke this function is still from within ember-cli-build, especially as at the moment embroider/macros is still coupled to the app instance as the key for configs.store is the most used import for typescript apps and addons, and changing it would be difficult. Especially because it would mean changing it for our own addon which also imports and extends it (
ember-data
) and sets it as an app re-export. If it even were possible, it would be far more confusing for end users to navigate.In addition to this, it would require a lot of code duplications as javascript does not allow changing a class into a subclass, and introduce the potential for excess churn as we do have motivations for changing the import in the future, a change that if/when it occurs will have to be carefully coordinated and rolled out over an extensive period of time for the same reasons listed above.