[DEPRECATION RFC ember-data] deprecate errors changing record state

[runspired]

Rendered

[sandstrom]

Great suggestion! 🏅

How about adding these methods on the model itself, i.e. model.addErrors etc?

To me it would feel more consistent with the current API, for example accessing relationships are via model.belongsTo (it's not model.relationships.belongsTo).

(model.errors would still exist, but would be read-only, apart from the methods that already are on there, but they will probably be deprecated at some point, I assume)

[lindyhopchris]

This uses the singular whereas the design uses the plural.

[hjdivad]

@sandstrom i'd rather not scope creep into changing the API itself; the goal of this rfc is to follow-through on the (now quite old) deprecation and make it actionable for users.

---

On the question of moving the API to the model, I'd prefer to avoid adding more API surface area to a class users already subclass a lot.

To me it would feel more consistent with the current API, for example accessing relationships are via model.belongsTo (it's not model.relationships.belongsTo).

This is a good point about consistency although i think the safer way to consolidate this would be to move belongsTo &c. to record.relationships rather than to have a larger API on DS.Model.

[sandstrom]

@hjdivad Alright, you have a point regarding scope creep and the downside of adding more methods to model.

Another concern is that the method names model.errors.addErrors feels verbose (this is also mentioned in the RFC "The new method names may feel unnecessarily verbose"). Moving the API would allow less verbose method names, but since we're not going down that path, how about another solution:

Add a flag somewhere (suggestions below) and when that flag is enabled, we use the new API. That way we can keep the cleaner api model.errors.add (and it's also a better fit among the existing methods, e.g. model.errors.has).

Places we could add a flag

1. In the global config/environment.js
2. On the DS.Errors object (reopen and use isNewErrorMethodsAPI: true (similar to isNewSerializerAPI:true[1]).
3. On the methods themselves, add a third argument for API opt-in errors.add(…, …, true) and in 3.9 we'd make the new API default and drop the third argument.

(I'd prefer (3), but either would work)

[1] https://www.emberjs.com/blog/2015/06/18/ember-data-1-13-released.html#toc_upgrade-guide

@hjdivad @runspired What's your thoughts?

[hjdivad]

@sandstrom I do like the idea of a flag rather than moving the methods. But the downside is the flag is all or nothing: in a large codebase it can be important to be able to migrate deprecations incrementally.

Adding a flag to the call site does allow for incremental migration and from my point of view is roughly equivalent to making a new method. I don't have a strong opinion between the two choices. As i see it, each has a slight edge over the other:

1. Adding a flag creates more work for people migrating as they add the flag and then later should drop it for cleanup once the deprecation is removed
2. But creating less pleasant method names impacts new projects

I could go either way (although again I'm not a fan of a global flag due to the impact on incremental conversion).

@runspired @dgeb ?

[runspired]

@sandstrom @hjdivad

I agree with David on being opposed to a global flag. I am additionally against the local flag and in favor of the changes here as proposed for three reasons. I list them here in order of importance:

Assuming a local opt-in API like the following:

add(member: string, message: string|string[], optIntoFuture: boolean)

1. My largest complaint with this syntax is that it leads to a double deprecation lifecyle.

Not only would users have to opt-in at each call site to eliminate the deprecation and ensure their code did not break, but once the deprecation completed they would need to receive a new deprecation telling them to drop the third argument.

2. & 3. My second and third complaints with this syntax both pertain to believing that the syntax today is misleading. add remove and clear are evocative of the set delete and clear APIs of Map and WeakMap. Given their behaviors and the nature of the data structure (a mapping of errors), one might expect the APIs to behave similarly, but they don't.

set(key, value) doesn't differentiate between value being an array vs not, but add/addErrors does.

delete(key) doesn't leave behind an empty array, but remove/removeErrors does. Also note, that while you can add a single error to an attribute, remove removes all of them, not just the one.

clear is the only API of the three that is semantically the same.

Both because I dislike the confusion with the mechanics of true Map classes, and because of the differences in function signatures, the names of the existing APIs make me uneasy.

[sandstrom]

@runspired Good arguments!

1. In my opinion we could skip the second deprecation, since the third argument will be ignored. But I understand if you'd like to do it "the right way" and make it a two-phase thing.

2. Not really sold on the confusion risk with map, I think people will understand that add on different objects will have different effects.

That said, this is only the voice of one person and if you, as the RFC champion, disagree I think we should go ahead with this PR anyway. I've declared my suggestions above and they've been discussed, which is the important part — if none of them end up in the final version (i.e. discarded) that's totally fine! 😄

So even though I have some concerns (1 & 2) I'm still overall positive to this RFC! 🏅

[hjdivad]

Moving this to final comment period. We'll aim to merge next week, barring any additional concerns.

Thanks very much @sandstrom @lindyhopchris for taking the time to review and give feedback. 🎉

[runspired]

We landed a different approach to resolving this due to a desire to reconsider how we propagate errors through the system more holistically.