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.

Sign up for GitHub

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-model-reopen #738

Merged
merged 1 commit into from
May 6, 2021
Merged

rfc: ember-data | deprecate-model-reopen #738

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
57 changes: 57 additions & 0 deletions text/0738-ember-data-deprecate-model-reopen.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
---
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/738
---

# EmberData | Deprecate Model Reopen

## Summary

Deprecates using `reopen` to alter clases extending `@ember-data/model`.

## Motivation

`reopen` restricts the ability of EmberData to design better primitives that maintain
compatibility with `@ember-data/model`, and is a footgun that leads to confusing incorrect
states for users that utilize it.

## Detailed design

The static `reopen` method on `Model` will be overwritten to provide a deprecation which
once resolved or after the deprecation lifecycle completes will result in `reopen` throwing
an error when used in dev and silently no-oping in production. This deprecation will target
`5.0` and not be `enabled` sooner than `4.1` though it may come available before that.

## How we teach this

This is best taught through a deprecation guide. Users using `reopen` to test multiple
configurations in their test suite should instead extend and register a new model each time.

Users using `reopen` to modify a class immediately after creating it should also refactor
to `extend` instead.

Users using `reopen` to modify a class dynamically at runtime should refactor to either register
new model types or (better) utilize a megamorphic solution such as ember-m3 to achieve their needs.

In all cases, using `reopen` after a class instance for a record has already been created has *always*
resulted in at least minor and potentially major errors in application state.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we could expand on this? Are examples you have seen involve messing with some of the Model states? Overall, I think we can help user's understand a specific case where reopen may have lead them astray.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically there are three issues though I don't think they need to be laid out here in the rfc text itself:

  1. is that we cache the metas for relationships and attributes, so if a reopen were to redefine a relationship (or add a potential resolvable inverse to a relationship where none was before) then those definitions would likely not get applied to new instances as expected (we'd use old metas anywhere we pulled from cache which is most places).

  2. is that existing instantiated relationships don't get reconfigured which means that if their definition has been changed they are now potentially a significantly different shape.

  3. is a subset of (1). Serialization and normalization typically breaks because eachAttribute/eachRelationship will often be missing necessary definitions.


## Drawbacks

Test suites, including EmberData's own, that make use of `reopen` are often "order dependent" in order
for the test suite to pass, and refactoring them can sometimes be a difficult exercise in determining
which tests had modified the class to achieve the model shape needed by another test. In general though
the drawbacks here are small given the widespread adoption of class syntax and growing adoption of octane
paradigms.

## Alternatives

- Don't deprecate `reopen` and wait to replace `@ember-data/model` in it's entirety. This alternative would prevent us from providing custom decorators not extending from `computed` and limit potential build tools allowing users to optimize existing usage of EmberData.

- Wait for `ember` to deprecate `reopen`. Because we cannot build custom decorators and support `reopen` without asking for `ember` to make available to use private APIs I do not think we should wait for Ember here. This RFC does not preclude or force Ember to deprecate reopen more broadly.