Skip to content
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 4 commits into from
Jun 14, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 109 additions & 0 deletions text/1026-ember-data-deprecate-store-extends-ember-object.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
---
stage: accepted
start-date: # In format 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 Ember the framework.
runspired marked this conversation as resolved.
Show resolved Hide resolved

## 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.
runspired marked this conversation as resolved.
Show resolved Hide resolved

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
}
}
});
Comment on lines +56 to +62
Copy link
Member

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? 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have we considered any of the negative aspects of introducing a new way to turn off deprecations like this?

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

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 🤔

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.

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)

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.

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

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.

```

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.
runspired marked this conversation as resolved.
Show resolved Hide resolved

## Drawbacks

none

## Alternatives

- deprecate every classic method to help folks find usage
- not chosen as its 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 user's migrate by swapping import
locations.
- not chosen as this is too minimal a change
runspired marked this conversation as resolved.
Show resolved Hide resolved

## Unresolved questions

None
Loading